feat(repo): Make gettoken callable outside of react #7325
Conversation
|
📝 WalkthroughWalkthroughThis pull request introduces a new Pre-merge checks❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Disabled knowledge base sources:
⛔ Files ignored due to path filters (2)
📒 Files selected for processing (9)
🧰 Additional context used📓 Path-based instructions (13)**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Files:
**/*.{js,jsx,ts,tsx,json,md,yml,yaml}📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Files:
packages/**/src/**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Files:
**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Files:
packages/**/src/**/*.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Files:
**/*.ts?(x)📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Files:
**/index.ts📄 CodeRabbit inference engine (.cursor/rules/typescript.mdc)
Files:
**/*.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Files:
**/*.{js,ts,jsx,tsx,json,md,yml,yaml}📄 CodeRabbit inference engine (.cursor/rules/monorepo.mdc)
Files:
**/*⚙️ CodeRabbit configuration file
Files:
**/*.{test,spec}.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Files:
**/*.{test,spec,e2e}.{ts,tsx,js,jsx}📄 CodeRabbit inference engine (.cursor/rules/development.mdc)
Files:
🧬 Code graph analysis (2)packages/shared/src/getToken.ts (3)
packages/shared/src/__tests__/getToken.spec.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (27)
🔇 Additional comments (1)
Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
bratsos
changed the title
|
@bratsos fyi people wanted to use this in expo (web/mobile) as well :) |
Hm, the issue here is that our solution depends on |
packages/shared/src/getToken.ts
Outdated
| if (!clerk.session) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
I think null is appropriate here, but wanted to flag just in case: Do you think we should throw an error in this case so the developer knows that a user is signed out? I'm debating both approaches and I'm not totally positive which one might be better. Leaning towards null...
There was a problem hiding this comment.
I think the question we need to answer is what is the responsibility of this function, to report the auth state, or to provide a token? If the answer is to report the auth state (which is what I think the intention was by using getX), then returning null seems the better option.
Plus we don't want to penalize the happy path by forcing devs to always have to wrap this with a try/catch.
There was a problem hiding this comment.
I was one of the people asking for this, as it would make things nicer in our own dashboard, but thinking about this again now, I think it might be problematic to introduce this API and I'm sorry I didn't realize it earlier. 😞
The reason is that I think this is fundamentally incompatible with any concurrent features in frameworks, like transitions in React. If we let people access this directly, they kind of bypass any SDK integrations we design to work well with transitions.
If we'd for example leverage transitions to get rid of our transitive state in React, if users were using this we would not be able to control the token they see based on the transition. We want the current page to keep the old token, and the new page to use the new token, but this helper would always return whatever the current value is.
We are still some ways away from being able to fully support concurrent features, but I think this PR would introduce an API that could lead users away from the happy path and make that harder for us to do in the future. At least I think there might be enough uncertainty whether this is a good idea long term that we should hold it until we know for sure?
Another way to say this is, in the framework SDKs we usually lead users away from relying on the clerk singleton directly, as that bypasses the SDK layer we've designed for this framework, which can be unsafe. I think this is another version of that?
There was a problem hiding this comment.
I've been thinking about this some more and I'm not sure we can ever make auth state fork since cookies are involved. If we decide that auth state ultimately needs to be synchronous state, this PR still makes sense. I'm not sure we're in a place to decide either way just yet though, but wanted to update on my thinking. 😄
There was a problem hiding this comment.
Thanks for the thoughtful feedback @Ephem!
So if I understand this correctly, this is an existing issue as:
- users can access
window.Clerk.session.getTokendirectly - users can access
_sessioncookie directly createGetTokenin our react hooks is accessingisomorphicClerk.session.getToken(options)which is also not concurrency-safe
We could ship this helper for the intended use cases (axios interceptors, react-query etc) that run outside React's render phase, with clear documentation that framework-specific hooks should be preferred for use within components.
Re:concurrent features and transitions, even though it's not part of this implementation you raised a great point. Just to test my understanding, wouldn't the following be sufficient to handle this (as a future iteration, of course):
- use
useSyncExternalStorefor our react hook to safely get the current session - introduce a way to maintain "versioned state generations" to handle transitions where different renders need different token values
There was a problem hiding this comment.
So, this is tricky, but I'll try to provide some more details around my current thinking. Happy to hop on a call as well.
users can access window.Clerk.session.getToken directly
Yes, but this is something we explicitly advice against. Same with useClerk. When you use those, you step outside of approved path and there be dragons.
users can access _session cookie directly
Accessing this directly in code is also advised against (I think? should be), but it's accessed implicitly when doing same-domain API calls, which is the main problem for supporting transitions, see later comment.
createGetTokenin our react hooks is accessingisomorphicClerk.session.getToken(options)which is also not concurrency-safe
Yes, today this is the case. Because it's behind our React hook, we can transparently change this implementation if we want though, which is my point. The hooks are a React-level facade that guarantees we can use React-level features in the future, even if we are not today.
We could ship this helper for the intended use cases (axios interceptors, react-query etc) that run outside React's render phase, with clear documentation that framework-specific hooks should be preferred for use within components.
This wont help unfortunately. Since react-query etc can be triggered from within the render phase anyway (when using suspense), this getToken implementation would still be tied to the render phase and the transition that triggered it. If people start using this for example with RQ, and we later want to update getToken to be transition-compatible via some React-level API, we couldn't do it since they are relying on this outside function.
All this said, the tricky thing here is the cookie. Even if we could fork the state itself, the cookie is not forkable. The transition that uses the "new" state might do suspenseful data fetching during the render phase, so it would need the cookie to already be set to do so. If we set the cookie when triggering the transition, we run into the problem that any synchronous interrupt that might happen that triggers new data fetching in the current state fork of the world now has the wrong cookie value. This is all from the point of view of our current model of dealing with all this on the client.
If we consider the canonical way to do this in a RSC framework, that's likely by moving the auth state change to a server function. These are not quite synchronous, as the request itself is async and the app can still update while they are in progress (with existing state), but they happen in serial and they apply kinda synchronously in that auth state and cookie and UI would update together. More research is needed though and I'm not sure how this would affect this PR and new API. 🤔
There was a problem hiding this comment.
Thanks for the detailed response! I want to make sure we're aligned on what this is trying to solve.
I think we might be approaching the "users can access window.Clerk directly" point from different angles. My thinking is that these users already exist - people using vanilla JS, or non-concurrent frameworks - and they're already accessing window.Clerk directly or reading cookies because there's no other option for them. They'll likely continue to exist even with better RSC auth support. Right now the experience involves implementing their own timing/retry logic to wait for Clerk to load.
This PR is really about providing a shared SDK utility for those users, rather than replacing framework-specific hooks as the recommended approach.
On the React hooks being a facade - I think we're actually in agreement here! Having a framework-agnostic getToken() wouldn't mean our React SDK has to use it internally. The React hooks could continue to have their own implementation that evolves toward transition support, using useSyncExternalStore or whatever approach makes sense. The standalone function would serve non-React use cases, while useAuth().getToken() remains the recommended path for React users.
Regarding the concern about React Query users - I wonder if those users would have used our hooks anyway, or if they're the same folks already using window.Clerk today.
It also feels like requiring a component tree structure to access something that's globally available might be more friction than necessary for cases like configuring an Axios interceptor at app initialization.
I guess my main question is: is the concern primarily about encouraging React users away from hooks (which could potentially be addressed through documentation), or is there a deeper architectural issue with having a non-concurrent-aware getToken exist alongside the React implementation?
Happy to dig into either - I just want to make sure we're solving for the right thing.
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@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: |
bratsos
force-pushed
the
alexbratsos/user-4045-make-gettoken-callable-outside-of-react
branch
from
a3303fc to
8938fb2
Compare
bratsos
force-pushed
the
alexbratsos/user-4045-make-gettoken-callable-outside-of-react
branch
from
8938fb2 to
ea3ef70
Compare
bratsos
force-pushed
the
alexbratsos/user-4045-make-gettoken-callable-outside-of-react
branch
from
7733626 to
41a623b
Compare
nikosdouvlis
force-pushed
the
vincent-and-the-doctor
branch
from
d24d455 to
12a12f5
Compare
bratsos
force-pushed
the
alexbratsos/user-4045-make-gettoken-callable-outside-of-react
branch
from
41a623b to
46efb2c
Compare
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
getTokenAPI across Astro, Next.js, Nuxt, React, React Router, TanStack React Start, and Vue SDKs for retrieving the current session token in the browser with automatic Clerk initialization handling.Tests
getTokencovering multiple Clerk states, loading scenarios, error conditions, and edge cases.✏️ Tip: You can customize this high-level summary in your review settings.