-
Notifications
You must be signed in to change notification settings - Fork 253
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
feat(clerk-expo): Support swapping publishableKey at runtime #1655
Conversation
🦋 Changeset detectedLatest commit: 5150098 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 |
!snapshot |
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.
✅ Great news! Jit hasn't found any security issues in your PR. Good Job! 🏆
Hey @BRKalow - the snapshot version command generated the following package versions:
Tip: use the snippet copy button below to quickly install the required packages. # @clerk/backend
npm i @clerk/backend@0.28.0 # @clerk/chrome-extension
npm i @clerk/chrome-extension@0.3.29-snapshot.46d14e7 # @clerk/clerk-js
npm i @clerk/clerk-js@4.56.2-snapshot.46d14e7 # eslint-config-custom
npm i eslint-config-custom@0.3.0 # @clerk/clerk-expo
npm i @clerk/clerk-expo@0.18.20-snapshot.46d14e7 # @clerk/fastify
npm i @clerk/fastify@0.6.4 # gatsby-plugin-clerk
npm i gatsby-plugin-clerk@4.4.6-snapshot.46d14e7 # @clerk/localizations
npm i @clerk/localizations@1.25.0 # @clerk/nextjs
npm i @clerk/nextjs@4.23.4-snapshot.46d14e7 # @clerk/clerk-react
npm i @clerk/clerk-react@4.24.1-snapshot.46d14e7 # @clerk/remix
npm i @clerk/remix@2.10.1-snapshot.46d14e7 # @clerk/clerk-sdk-node
npm i @clerk/clerk-sdk-node@4.12.3 # @clerk/shared
npm i @clerk/shared@0.22.0 # @clerk/themes
npm i @clerk/themes@1.7.5 # @clerk/types
npm i @clerk/types@3.50.0 |
@@ -256,6 +256,7 @@ export default class Clerk implements ClerkInterface { | |||
|
|||
const { frontendApi, instanceType } = publishableKey as PublishableKey; | |||
|
|||
this.publishableKey = key; |
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.
This is specified as a public readonly property, but we weren't setting it anywhere.
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'm almost sure this is not used anywhere - @panteliselef , any chance this is needed by the multidomains feature?
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'm using it in another spot in this PR to check for a new key, but let me know if we don't want to set this for some reason!
// This method should be idempotent in both scenarios | ||
if (!inClientSide() || !this.#instance) { | ||
if (!inClientSide() || !this.#instance || (options.Clerk && this.#instance.Clerk !== options.Clerk)) { |
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.
Without this change, there was no way to forcibly reset the created IsomorphicClerk
instance. Given that we have not considered the possibility of changing the instance of Clerk
at runtime, adding this additional check seems reasonable given the new requirement.
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.
This makes sense 👍🏻
@@ -25,6 +25,8 @@ export function ClerkProvider(props: ClerkProviderProps): JSX.Element { | |||
return ( | |||
//@ts-expect-error | |||
<ClerkReactProvider | |||
// Force reset the state when the provided key changes, this ensures that the provider does not retain stale state. See JS-598 for additional context. | |||
key={key} |
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 think that there is publishableKey
and not key
prop in ClerkReactProvider
. Could we double check this?
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.
The use of key here is intentional to force react to remount the component and wipe away any stale state from the previous instance. As far as I can tell, by passing the clerk instance below we remove the need to pass the publishableKey as a prop
This PR has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Type of change
Packages affected
@clerk/clerk-js
@clerk/clerk-react
@clerk/nextjs
@clerk/remix
@clerk/types
@clerk/themes
@clerk/localizations
@clerk/clerk-expo
@clerk/backend
@clerk/clerk-sdk-node
@clerk/shared
@clerk/fastify
@clerk/chrome-extension
gatsby-plugin-clerk
build/tooling/chore
Description
npm test
runs as expected.npm run build
runs as expected.Support swapping the
publishableKey
at runtime, and by extension the Clerk instance. This allows use cases such as a runtime environment switcher to allow native app developers to toggle their runtime Clerk instance without force quitting an app or having two separate builds. See JS-598 or #1508 for additional details.fixes JS-598, fixes #1508