-
Notifications
You must be signed in to change notification settings - Fork 206
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(clerk-js): Bundle the zxcvbn
library inside the headless variant
#3038
base: main
Are you sure you want to change the base?
fix(clerk-js): Bundle the zxcvbn
library inside the headless variant
#3038
Conversation
🦋 Changeset detectedLatest commit: b3e55f8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@anagstef Let's add an e2e here that uses the headless entry and calls |
@anagstef I think we should also mention the increase in the bundle size as it is significant. |
@BRKalow we should find a way to mimic the non-browser environment. We should give a shot to https://wix.github.io/Detox/
@panteliselef you mean on the changeset? |
packages/clerk-js/webpack.config.js
Outdated
new webpack.DefinePlugin({ | ||
__IS_BROWSER__: variant !== variants.clerkHeadless, | ||
}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that this is only used in clerkHeadless
package, so let's kill the variant
argument from here and move the plugin definition to the clerkHeadless rules in prodConfig
(probably line 196?)
let core; | ||
let languageCommon; | ||
if (__IS_BROWSER__) { | ||
core = import('@zxcvbn-ts/core'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Is this a sync import?
Pretty sure this is not widely supported yet - this should probably be await
ed here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a webpack config we can leverage where dynamic imports are "ignored" and instead they are bundled as normal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a sync import?
This is followed by:
return Promise.all([core, languageCommon]);
so, we don't expect it to be sync.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a webpack config we can leverage where dynamic imports are "ignored" and instead they are bundled as normal?
Didn't find anything. :(
This will fix the usage of the `validatePassword` on Expo.
00b4f82
to
404a197
Compare
core = await require('@zxcvbn-ts/core'); | ||
languageCommon = await require('@zxcvbn-ts/language-common'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't require synchronous ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anagstef I also think that the await
before the require isn't doing anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I've removed this, but haven't pushed yet! Thank you both for noticing!
core = await import('@zxcvbn-ts/core'); | ||
languageCommon = await import('@zxcvbn-ts/language-common'); | ||
} | ||
return Promise.all([core, languageCommon]); | ||
return { core, languageCommon }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean the the dependencies will no longer load in parallel ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Nice catch! I'll revert this one.
@anagstef do we have any updates on this one? |
Description
Currently, the
zxcvbn
is lazy loaded on clerk-js due to its size. However, usingimport()
is not working in non-browser environments, including Expo.Considering the assumption that the
clerk.headless.js
variant is only used on native environments, this PR bundles the wholezxcvbn
inside the headless clerk-js.This PR will fix the usage of the
validatePassword
on Expo.Checklist
npm test
runs as expected.npm run build
runs as expected.Type of change