poc: Remove contexts and subscribe to clerk changes directly in hooks #7267
Conversation
…uth instead of at the ClerkProvider level
… react package to use it
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Comment |
| this.#listeners.push(listener); | ||
| // emit right away | ||
| if (this.client) { | ||
| if (this.client && !options?.skipInitialEmit) { |
There was a problem hiding this comment.
uSES reads state directly from clerk, so this was introduced to avoid extra emits every time addListener was called in a hook.
| const propsWithEnvs = mergeNextClerkPropsWithEnv({ | ||
| ...rest, | ||
| initialState: statePromiseOrValue as InitialState | Promise<InitialState> | undefined, | ||
| nonce: await noncePromiseOrValue, |
There was a problem hiding this comment.
Awaiting this is something we might also want to move further down.
There was a problem hiding this comment.
This should likely be renamed now that it doesn't actually contain a context.
|
|
||
| export { ClerkProvider }; | ||
|
|
||
| const useLoadedIsomorphicClerk = (options: IsomorphicClerkOptions) => { |
There was a problem hiding this comment.
This was moved up from the now deleted ClerkContextProvider, but code is unchanged.
There was a problem hiding this comment.
Diff from the old PR, ignore. I haven't at all touched the tests for this PoC.
| if (initialState && 'then' in initialState) { | ||
| // TODO: If we want to preserve backwards compatibility, we'd need to throw here instead | ||
| // @ts-expect-error See above | ||
| return React.use(initialState); | ||
| } |
There was a problem hiding this comment.
If initialState is passed in as a Promise, this is going to make sure any hook that relies on it suspends at the location of the hook. Before, you would always have to await at the provider level.
This should not be a breaking change.
There was a problem hiding this comment.
Does this mean we are requiring React@19? I believe use() is not available in 18
There was a problem hiding this comment.
The comment mentions this but is not super clear, but taking this all the way would mean we refactor this to throw the promise instead for backwards compatibility. That's a bit more work to implement though so went with use just for the poc. 😄
If we want to use use and avoid introducing a legacy React way to do this, maybe we could instead error if passing in initialState as a Promise on React < 19?
Edit: Since we own the Next package that will likely be the first and for a while the only user of this, we could either await initialState or pass down the promise directly depending on the Next version. Pre 16 we await it like today, >16 we pass down the promise.
|
|
||
| const propsWithEnvs = mergeNextClerkPropsWithEnv({ | ||
| ...rest, | ||
| initialState: statePromiseOrValue as InitialState | Promise<InitialState> | undefined, |
There was a problem hiding this comment.
Not awaiting this at the top level might be considered a breaking change. I'm not sure it is technically, but it might change the loading experience of apps, so we should be careful.
We could also do this incrementally and optionally by introducing something like dynamic="stream".
| // which for example means that already mounted <Suspense> boundaries might suddenly show their fallback. | ||
| // This makes all auth state changes into transitions, but does not deopt to be synchronous. If it's | ||
| // called during a transition, it immediately uses the new value without deferring. | ||
| return useDeferredValue(authState); |
There was a problem hiding this comment.
This is a common pattern for all the hooks and is a breaking change. Doing this by default in the react package might introduce transitions into apps that don't have them currently, which might cause problems, so we'll likely want to find a way to introduce this incrementally/optionally and let the framework level decide.
This does not by itself get rid of the transitive state. To do that safely we probably have to start the transition ourselves from inside clerk-js so we can make sure it wraps both the navigation and the state emit.
Description
This is a proof of concept that reimagines a few things. It builds on #7194 but takes things quite a bit further.
Note
I'm targeting this directly at
vincent-and-the-doctorand not the PR above. That means this PR contains some of the same changes as that one, but since it also reworks things quite a bit I choose to target it this way so this can be reviewed entirely stand alone.There are a lot of details in here, but in essence, what this PR does is:
ContextProvidersfromuiandreactintoshared/reactclerk.addListener()in that provider, also removes most contextsUserContextare no longer exported fromshared/reactclerkviauseSyncExternalStoreuseUserBaseare also reexported asuseUserContextto avoid breaking change for theseinitialStatecan now be a promise<ClerkProvider dynamic>in the Next package no longer doesawait initialStateChecklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change