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.
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.
❓ Is this a sync import?
Pretty sure this is not widely supported yet - this should probably be awaited here.
There was a problem hiding this comment.
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.
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.
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.
anagstef
force-pushed
the
stefanos/sdk-1545-validatepassword-throws-error-on-expo
branch
from
00b4f82
to
404a197
Compare
| core = await require('@zxcvbn-ts/core'); | ||
| languageCommon = await require('@zxcvbn-ts/language-common'); |
There was a problem hiding this comment.
Isn't require synchronous ?
There was a problem hiding this comment.
@anagstef I also think that the await before the require isn't doing anything
There was a problem hiding this comment.
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.
Does this mean the the dependencies will no longer load in parallel ?
There was a problem hiding this comment.
Hmm. Nice catch! I'll revert this one.
|
@anagstef do we have any updates on this one? |
Description
Currently, the
zxcvbnis 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.jsvariant is only used on native environments, this PR bundles the wholezxcvbninside the headless clerk-js.This PR will fix the usage of the
validatePasswordon Expo.Checklist
npm testruns as expected.npm run buildruns as expected.Type of change