-
Notifications
You must be signed in to change notification settings - Fork 403
fix(clerk-react): Correct useAuth().isLoaded in transitive state #7157
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?
Changes from all commits
10aab8f
67581b5
35b2865
eb81b28
77d7628
044d9ca
b358394
070b871
0f3f02f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@clerk/clerk-react': patch | ||
| --- | ||
|
|
||
| Ensure that useAuth() hook returns isLoaded=false when isomorphicClerk is loaded but we are in transitive state |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -99,13 +99,9 @@ export const useAuth = (initialAuthStateOrOptions: UseAuthOptions = {}): UseAuth | |
| const initialAuthState = rest as any; | ||
|
|
||
| const authContextFromHook = useAuthContext(); | ||
| let authContext = authContextFromHook; | ||
|
|
||
| if (authContext.sessionId === undefined && authContext.userId === undefined) { | ||
| authContext = initialAuthState != null ? initialAuthState : {}; | ||
| } | ||
|
Comment on lines
-104
to
-106
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, so previously it was falling back to the initial auth state when I think you might have stumbled on the root cause of the sign in bug here too! When signing in, the initial state would be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
|
||
| const isomorphicClerk = useIsomorphicClerkContext(); | ||
| const authContext = !isomorphicClerk.loaded && initialAuthState ? initialAuthState : authContextFromHook; | ||
Ephem marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just want to verify my understanding here. The reason this is safe is because:
Correct? (These are a lot of hidden assumptions/guarantees btw) What happens if we want to start making the auth state updates in transitions as per #6905? I think it might be possible we get a race condition where @panteliselef FYI ☝️
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are spot on regarding the assumptions. We can of course change how things work if we want to support transitions. |
||
|
|
||
| const getToken: GetToken = useCallback(createGetToken(isomorphicClerk), [isomorphicClerk]); | ||
| const signOut: SignOut = useCallback(createSignOut(isomorphicClerk), [isomorphicClerk]); | ||
|
|
||
|
|
||

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 test is passing even with the existing logic for me.
I think the prerequisite for the bug to happen is that we explicitly pass in
initialAuthState(like we do fromusePromisifiedAuth, which is the reason this is failing in Next).For a test that fails with the old code and pass with the new (might want to update the test description too):
We could also keep this test as is since it covers a new case we didn't have coverage for, and add a new test with the above.
Overall, I think several tests here might benefit from the test approach you are using here btw,
useAuthis the main public API so we should be testing that directly instead ofuseDerivedAuth.(One could also argue we should move/duplicate this test to
usePromisifiedAuthsince that's really the broken public API here, but I don't think we should do that right now)