-
Notifications
You must be signed in to change notification settings - Fork 426
feat(ui,react): Add shared React variant to reduce bundle size #7601
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 83fc89f The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughAdds a shared React build variant for 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
Introduces a "shared" variant of @clerk/ui that externalizes React dependencies, allowing the host application's React to be reused instead of bundling a separate copy. Changes: - Add @clerk/ui/register module to register React on globalThis - Add ui.shared.browser.js build variant with externalized React - Add React version compatibility checking in @clerk/react - Add clerkUiVariant option to load the appropriate variant - Make dev server React externalization conditional via --env shared Co-Authored-By: Claude Opus 4.5 <[email protected]>
Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Extract version checking logic into testable utility (versionCheck.ts) - Add comprehensive unit tests for version parsing and bounds checking - Move version compatibility check to module level for better performance - Add dev warning when pnpm-workspace.yaml fallback is used - Add warning for React version mismatch in register modules - Remove redundant clerkUiVariant assignment in isomorphicClerk.ts Co-Authored-By: Claude Opus 4.5 <[email protected]>
The new exports field in clerk-js/package.json blocked deep imports that expo and chrome-extension packages depended on. This adds: - ./internal/fapi export for FapiRequestInit/FapiResponse types (expo) - ./no-rhc export for the no-RHC variant (chrome-extension) Also updates expo to use the new clean import path. Co-Authored-By: Claude Opus 4.5 <[email protected]>
- Rename register/index.js to register/index.cjs to fix module syntax mismatch (package uses "type": "module" but file was CommonJS) - Add register/index.d.cts for CJS type declarations to fix "Masquerading as ESM" attw error - Update exports to point to correct file extensions - Run pnpm dedupe to clean up lockfile Co-Authored-By: Claude Opus 4.5 <[email protected]>
The new exports field introduces ESM entry points (.mjs) with CJS type declarations (.d.ts), which triggers attw's "Masquerading as CJS" warning. This is expected behavior for this package's build setup, so we ignore the false-cjs rule. Co-Authored-By: Claude Opus 4.5 <[email protected]>
This reverts commit 74be850.
…rhc variant" This reverts commit 364059b.
f50f25d to
d7c670c
Compare
Add required curly braces after if conditions in versionCheck.ts and fix import sorting in index.ts. Co-Authored-By: Claude Opus 4.5 <[email protected]>
bratsos
left a comment
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.
I haven't run it locally yet, but I did a deep dive in the code and it looks good! That's a great optimization 🚀
I'll run it locally on Monday if it's not merged yet as a sanity check.
|
Love it! Are we tracking the routing logic for this variant in the sdk-infra-workers repo? |
|
@jacekradko nope, I'll take a look! |
|
@jacekradko On second thought, I'm not sure we need additional logic in the proxy...we don't need/want the ability to pin specific domains to this variant necessarily, it's something that's internal to the SDK |
|
!snapshot |
|
Hey @brkalow - the snapshot version command generated the following package versions:
Tip: Use the snippet copy button below to quickly install the required packages. npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact |
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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/ui/rspack.config.js`:
- Around line 20-36: The externals handler sharedReactExternalsHandler currently
externalizes 'react' and 'react-dom' but misses subpaths like 'react-dom/client'
and 'react-dom/server', so update the function (the sharedReactExternalsHandler
that takes ({ request }, callback)) to also detect requests equal to
'react-dom/client' and 'react-dom/server' and call callback(null,
['__clerkSharedModules', 'react-dom/client'], 'root') and callback(null,
['__clerkSharedModules', 'react-dom/server'], 'root') respectively (keeping the
existing branches for 'react' and 'react-dom' and leaving callback() as the
default).
| /** | ||
| * Externals handler for the shared variant that reads React from globalThis.__clerkSharedModules. | ||
| * This allows the host application's React to be shared with @clerk/ui. | ||
| * @type {import('@rspack/core').ExternalItemFunctionData} | ||
| */ | ||
| const sharedReactExternalsHandler = ({ request }, callback) => { | ||
| if (request === 'react') { | ||
| return callback(null, ['__clerkSharedModules', 'react'], 'root'); | ||
| } | ||
| if (request === 'react-dom') { | ||
| return callback(null, ['__clerkSharedModules', 'react-dom'], 'root'); | ||
| } | ||
| if (request === 'react/jsx-runtime') { | ||
| return callback(null, ['__clerkSharedModules', 'react/jsx-runtime'], 'root'); | ||
| } | ||
| callback(); | ||
| }; |
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.
🧩 Analysis chain
🏁 Script executed:
rg -nP "react-dom/(client|server)" packages/ui -g "*.{js,jsx,ts,tsx}"Repository: clerk/javascript
Length of output: 146
🏁 Script executed:
cat -n packages/ui/rspack.config.js | head -50Repository: clerk/javascript
Length of output: 2177
🏁 Script executed:
cat -n packages/ui/rspack.config.js | sed -n '220,250p'Repository: clerk/javascript
Length of output: 1263
🏁 Script executed:
cat -n packages/ui/rspack.config.js | sed -n '285,310p'Repository: clerk/javascript
Length of output: 865
Externalize react-dom/client in the shared variant to prevent React version mismatches.
The codebase imports react-dom/client in packages/ui/src/lazyModules/common.ts, but the sharedReactExternalsHandler only covers react-dom, not its subpaths. This means react-dom/client will be bundled separately instead of shared, causing a version mismatch with the externalized React at runtime.
Extend the handler to cover react-dom/client and react-dom/server:
🔧 Patch
const sharedReactExternalsHandler = ({ request }, callback) => {
if (request === 'react') {
return callback(null, ['__clerkSharedModules', 'react'], 'root');
}
- if (request === 'react-dom') {
+ if (request === 'react-dom' || request === 'react-dom/client' || request === 'react-dom/server') {
return callback(null, ['__clerkSharedModules', 'react-dom'], 'root');
}
if (request === 'react/jsx-runtime') {
return callback(null, ['__clerkSharedModules', 'react/jsx-runtime'], 'root');
}
callback();
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Externals handler for the shared variant that reads React from globalThis.__clerkSharedModules. | |
| * This allows the host application's React to be shared with @clerk/ui. | |
| * @type {import('@rspack/core').ExternalItemFunctionData} | |
| */ | |
| const sharedReactExternalsHandler = ({ request }, callback) => { | |
| if (request === 'react') { | |
| return callback(null, ['__clerkSharedModules', 'react'], 'root'); | |
| } | |
| if (request === 'react-dom') { | |
| return callback(null, ['__clerkSharedModules', 'react-dom'], 'root'); | |
| } | |
| if (request === 'react/jsx-runtime') { | |
| return callback(null, ['__clerkSharedModules', 'react/jsx-runtime'], 'root'); | |
| } | |
| callback(); | |
| }; | |
| /** | |
| * Externals handler for the shared variant that reads React from globalThis.__clerkSharedModules. | |
| * This allows the host application's React to be shared with `@clerk/ui`. | |
| * `@type` {import('@rspack/core').ExternalItemFunctionData} | |
| */ | |
| const sharedReactExternalsHandler = ({ request }, callback) => { | |
| if (request === 'react') { | |
| return callback(null, ['__clerkSharedModules', 'react'], 'root'); | |
| } | |
| if (request === 'react-dom') { | |
| return callback(null, ['__clerkSharedModules', 'react-dom'], 'root'); | |
| } | |
| if (request === 'react-dom/client') { | |
| return callback(null, ['__clerkSharedModules', 'react-dom/client'], 'root'); | |
| } | |
| if (request === 'react-dom/server') { | |
| return callback(null, ['__clerkSharedModules', 'react-dom/server'], 'root'); | |
| } | |
| if (request === 'react/jsx-runtime') { | |
| return callback(null, ['__clerkSharedModules', 'react/jsx-runtime'], 'root'); | |
| } | |
| callback(); | |
| }; |
🤖 Prompt for AI Agents
In `@packages/ui/rspack.config.js` around lines 20 - 36, The externals handler
sharedReactExternalsHandler currently externalizes 'react' and 'react-dom' but
misses subpaths like 'react-dom/client' and 'react-dom/server', so update the
function (the sharedReactExternalsHandler that takes ({ request }, callback)) to
also detect requests equal to 'react-dom/client' and 'react-dom/server' and call
callback(null, ['__clerkSharedModules', 'react-dom/client'], 'root') and
callback(null, ['__clerkSharedModules', 'react-dom/server'], 'root')
respectively (keeping the existing branches for 'react' and 'react-dom' and
leaving callback() as the default).
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.
@brkalow I saw you explicitly put react-dom/client on __clerkSharedModules, so I'm guessing this is valid feedback and it needs to be added here as well?
Update: I missed that this was already addressed (apparently so did Coderabbit). 😅
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.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
Externalize react-dom/client in the shared variant and register it on globalThis.__clerkSharedModules so host apps can share it with @clerk/ui. Co-Authored-By: Claude Opus 4.5 <[email protected]>
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.
Really nice work here! I tried the current (outdated) snapshot out with the dashboard a bit and it seems to work well.
ui.browser.js in the dash shrank from 36.1kb to 19.4kb which is a nice win. The rabbit might have a point about react-dom/client though, which might possibly move the needle a bit more?
Update: Oh, I had totally missed this was already addressed and Coderabbit just missed picking up on it. Will do a new snapshot and try it out.
|
!snapshot |
|
Hey @Ephem - the snapshot version command generated the following package versions:
Tip: Use the snippet copy button below to quickly install the required packages. npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact
npm i @clerk/[email protected] --save-exact |
|
Hmm, trying out this very latest snapshot, I'm not seeing any bundle size gains in a local prod build of the dashboard. Logging // This comes from logging it in the build:
const bounds: VersionBounds[] = [
[ 18, 0, -1, 0 ],
[ 19, 0, 0, 3 ],
[ 19, 1, 1, 4 ],
[ 19, 2, 2, 3 ],
[ 19, 3, 3, 0 ]
];
it('returns true for compatible versions', () => {
expect(isVersionCompatible('19.3.0-canary-f93b9fd4-20251217', bounds)).toBe(true);
});Not sure what's going on, I might very well be doing something wrong. 🤔 |
Summary
ui.shared.browser.jsbuild variant that externalizes React dependencies, allowing the host application's React to be reused instead of bundling a separate copy@clerk/ui/registermodule to register React onglobalThis.__clerkSharedModulesfor sharing with@clerk/uiclerkUiVariantoption to explicitly control which variant to use@clerk/reactusers, the shared variant is automatically detected and enabled for compatible React versionsfixes USER-4440
Test plan
@clerk/reactpackage@clerk/react🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.