fix(react): attach UI for preloaded Clerk instances#8594
Conversation
🦋 Changeset detectedLatest commit: 6e983d7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 7 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.
|
📝 WalkthroughWalkthroughThis PR refactors Clerk.load() to initialize ClerkUI via a guarded Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@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: |
| const nextOptions = this.#initOptions({ ...this.#options, ...options }); | ||
| this.#options = nextOptions; |
There was a problem hiding this comment.
This part worries me a bit. After the initial load, the options have always been immutable before, now they can theoretically change, but they aren't reactive so if we've read them anywhere already, those values might now be stale (imagine clerk-js loads -> components in the host app rely on some options via the hooks -> ui loads with other options).
Supporting options updating for real with reactivity would be a huge pain for no real gain.
I think a clean alternative is to lift this to an __internal_attachUi(ui) function that's only callable after the initial load?
Internal only to avoid increasing the API surface needlessly for an advanced feature only we are likely to use, could make it public if there is demand.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/shared/src/types/clerk.ts (1)
248-254: ⚡ Quick winDocument the new exported hook more fully.
Since this sits on the exported
Clerkinterface, please add a short summary plus@param/@returnsdetails instead of only@internal. That keeps the generated/public type surface self-explanatory even for internal hooks.As per coding guidelines, "All public APIs must be documented with JSDoc".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/types/clerk.ts` around lines 248 - 254, Add full JSDoc for the exported __internal_attachClerkUI property on the Clerk interface: replace the lone `@internal` tag with a short one-line summary describing its purpose, add `@param` entries for ClerkUI (ClerkUIConstructor | Promise<ClerkUIConstructor>) and options (ClerkOptions | undefined) explaining what each is and when to pass them, and add an `@returns` describing the return type (void). Keep the `@internal` tag if needed but ensure the summary and `@param/`@returns are present so the public type surface is self-explanatory; reference the __internal_attachClerkUI property, ClerkUIConstructor, and ClerkOptions when writing the docs.packages/react/src/__tests__/isomorphicClerk.test.ts (1)
246-279: ⚡ Quick winAdd a regression test for the preloaded-instance fallback path.
This only exercises the
__internal_attachClerkUIbranch.getEntryChunks()still falls back toclerk.load(options)when a preloaded Clerk instance does not expose that hook, so a sibling test for that older-instance path would better protect the compatibility contract here.As per coding guidelines, "Non-major releases in
packages/clerk-jsandpackages/uimust remain backwards-compatible with SDK versions already in the wild. Changes must not remove or rename anything that older SDK versions still call, as this will break production for those users."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react/src/__tests__/isomorphicClerk.test.ts` around lines 246 - 279, Add a sibling test that simulates a preloaded global Clerk instance which does NOT expose __internal_attachClerkUI so the IsomorphicClerk.getEntryChunks() fallback to calling clerk.load(options) is exercised; specifically, create a mockClerkInstance with loaded: true and a mock load method (but no __internal_attachClerkUI), assign it to global.Clerk, instantiate IsomorphicClerk, call mountUserButton and then await getEntryChunks(), and assert that loadClerkUIScript was called and that mock load was invoked (and __internal_attachClerkUI was not), ensuring the flow through IsomorphicClerk.getEntryChunks -> clerk.load is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/react/src/__tests__/isomorphicClerk.test.ts`:
- Around line 246-279: Add a sibling test that simulates a preloaded global
Clerk instance which does NOT expose __internal_attachClerkUI so the
IsomorphicClerk.getEntryChunks() fallback to calling clerk.load(options) is
exercised; specifically, create a mockClerkInstance with loaded: true and a mock
load method (but no __internal_attachClerkUI), assign it to global.Clerk,
instantiate IsomorphicClerk, call mountUserButton and then await
getEntryChunks(), and assert that loadClerkUIScript was called and that mock
load was invoked (and __internal_attachClerkUI was not), ensuring the flow
through IsomorphicClerk.getEntryChunks -> clerk.load is covered.
In `@packages/shared/src/types/clerk.ts`:
- Around line 248-254: Add full JSDoc for the exported __internal_attachClerkUI
property on the Clerk interface: replace the lone `@internal` tag with a short
one-line summary describing its purpose, add `@param` entries for ClerkUI
(ClerkUIConstructor | Promise<ClerkUIConstructor>) and options (ClerkOptions |
undefined) explaining what each is and when to pass them, and add an `@returns`
describing the return type (void). Keep the `@internal` tag if needed but ensure
the summary and `@param/`@returns are present so the public type surface is
self-explanatory; reference the __internal_attachClerkUI property,
ClerkUIConstructor, and ClerkOptions when writing the docs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e4fec411-6322-47aa-8d35-362959663505
📒 Files selected for processing (5)
packages/clerk-js/src/core/__tests__/clerk.test.tspackages/clerk-js/src/core/clerk.tspackages/react/src/__tests__/isomorphicClerk.test.tspackages/react/src/isomorphicClerk.tspackages/shared/src/types/clerk.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/react/src/isomorphicClerk.ts
- packages/clerk-js/src/core/clerk.ts
API Changes Report
Summary
@clerk/astroCurrent version: 3.3.2 Subpath
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/shared/src/types/clerk.ts (1)
1655-1655: ⚡ Quick winKeep internal caller unions aligned for type safety.
__internal_AttemptToEnableEnvironmentSettingParams['caller']now includes'ConfigureSSO', but__internal_EnableOrganizationsPromptProps['caller']still excludes it, so core currently needs a cast when forwarding this value. Add'ConfigureSSO'there as well to keep these internal contracts in sync.Proposed diff
export type __internal_EnableOrganizationsPromptProps = { onSuccess?: () => void; onClose?: () => void; } & { caller: | 'OrganizationSwitcher' | 'OrganizationProfile' | 'OrganizationList' + | 'ConfigureSSO' | 'useOrganizationList' | 'useOrganization'; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/types/clerk.ts` at line 1655, The union type for the internal caller enums is out of sync: __internal_AttemptToEnableEnvironmentSettingParams['caller'] now includes 'ConfigureSSO' but __internal_EnableOrganizationsPromptProps['caller'] does not, forcing casts when forwarding values. Update the declaration of __internal_EnableOrganizationsPromptProps['caller'] to include the 'ConfigureSSO' member (mirror the same union used by __internal_AttemptToEnableEnvironmentSettingParams) so both internal caller unions remain aligned and remove the need for casts.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/shared/src/types/clerk.ts`:
- Line 1655: The union type for the internal caller enums is out of sync:
__internal_AttemptToEnableEnvironmentSettingParams['caller'] now includes
'ConfigureSSO' but __internal_EnableOrganizationsPromptProps['caller'] does not,
forcing casts when forwarding values. Update the declaration of
__internal_EnableOrganizationsPromptProps['caller'] to include the
'ConfigureSSO' member (mirror the same union used by
__internal_AttemptToEnableEnvironmentSettingParams) so both internal caller
unions remain aligned and remove the need for casts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 5889aa1d-7d94-441b-a667-5cb09f88d630
📒 Files selected for processing (2)
packages/clerk-js/src/core/clerk.tspackages/shared/src/types/clerk.ts
2 participants
Fixes #8569.
When React's
IsomorphicClerkreused an already-loaded global Clerk instance, it skipped the UI chunk load and immediately replayed premounted UI invocations, so sign-in/sign-up etc. never rendered. Now we load the UI chunk and callclerk.loadagain on the existing instance, which short-circuits the resource fetch but attachesClerkUIvia the new#initClerkUIpath.Includes regression tests for both packages and a patch changeset.
Before:
After: