Skip to content
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

fix(clerk): make useAuth's loading prop track the Clerk client #7602

Closed
wants to merge 1 commit into from

Conversation

jtoar
Copy link
Contributor

@jtoar jtoar commented Feb 10, 2023

I think something like this is the fix for https://community.redwoodjs.com/t/redwood-v4-0-and-clerk-dev-cannot-render-component-as-a-sessions-already-exists-redirecting-to-the-home-url/4567/11.

The gist of the problem is in the "try" part of the try-catch block in useReauthenticate here:

try {
const userMetadata = await authImplementation.getUserMetadata()
if (!userMetadata) {
setAuthProviderState({
...notAuthenticatedState,
client: authImplementation.client,
})
} else {

Here's notAuthenticatedState for completeness:

const notAuthenticatedState = {
isAuthenticated: false,
currentUser: null,
userMetadata: null,
loading: false,
hasError: false,
}

What this means is that if we can't get userMetadata, we consider the auth provider to have loaded. loading is false. But this isn't how Clerk works. Clerk isn't finished loading until the client is on window object (I think).

The work-in-progress fix in this PR is simple, maybe too simple? It doesn't use React APIs. If we want to use React APIs, we should reach for Clerk's useAuth hook and destructure out the isLoaded prop. But that would require changing the API of createAuthImplementation et. al a bit (I think—we could just add custom logic to useReauthenticate since we officially support Clerk, but this would kind of go against all the work we did to decouple auth in the first place).

There's a couple more changes I'd like to make in this fix:

  • I'm not sure that we need ClerkLoaded at all in the web/src/auth.tsx template; I think it may actually be bad because it blocks rendering of its children till Clerk has loaded. Ideally the AuthProvider should handle that so the Router can display the whileLoadingAuth component

  • it sounds like Clerk's useAuth hook is newer and more recommended than it's useUser hook; maybe we should go with that?

    See https://clerk.dev/docs/reference/clerk-react/useauth:

    This new hook should be used instead of:

    • useSession() , to access getToken() or the sessionId
    • useUser() , to access getToken() for integrations
    • useClerk() , to access signOut()

@jtoar jtoar added release:fix This PR is a fix fixture-ok Override the test project fixture check labels Feb 10, 2023
@jtoar jtoar requested a review from Tobbe February 10, 2023 00:08
@Tobbe
Copy link
Member

Tobbe commented Feb 10, 2023

Great work @jtoar! Excellent debugging.

it sounds like Clerk's useAuth hook is newer and more recommended than it's useUser hook; maybe we should go with that?

The way I read their documentation, you should only switch from useUser to useAuth if you use getToken from useUser.

@jtoar
Copy link
Contributor Author

jtoar commented Feb 10, 2023

Closing in favor of #7608

@jtoar jtoar closed this Feb 10, 2023
@jtoar jtoar deleted the ds-fix-clerk-redirecting branch February 27, 2023 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fixture-ok Override the test project fixture check release:fix This PR is a fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants