Uh oh!
There was an error while loading. Please reload this page.
fix(clerk-js): Broadcast sign out to all tabs on long lived clients - #5133
Conversation
🦋 Changeset detectedLatest commit: 3cd011e The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Uh oh!
There was an error while loading. Please reload this page.
| // If this.session exists, then signOut was triggered by the current tab | ||
| // and should emit. Other tabs should not emit the same event again | ||
| const shouldSignOutSession = this.session && newSession === null; | ||
| if (shouldSignOutSession) { | ||
| this.#broadcastSignOutEvent(); | ||
| eventBus.dispatch(events.TokenUpdate, { token: null }); | ||
| } | ||
There was a problem hiding this comment.
Since we are no longer delete the client on sign out, this.session would always be null, causing broadcasting to never occur.
Also removing eventBus.dispatch(events.TokenUpdate, { token: null }) does not cause issues, because the code a few lines below will handle it appropriately.
| }); | ||
| await mainTab.po.expect.toBeSignedOut(); | ||
| await mainTab.po.expect.toBeSignedOut({ timeOut: 2 * 1_000 }); |
There was a problem hiding this comment.
Previously the test was passing after 40-50 seconds because either /touch or /tokens would force update the state.
| // Notify other tabs that user is signing out. | ||
| this.__internal_broadcastSignOutEvent(); |
There was a problem hiding this comment.
Explicitly broadcast inside signOut() instead of attempting to detect if setActive({ session: null}) should notify
| }); | ||
| }; | ||
| // TODO: Deprecate this one, and mark it as internal. Is there actual benefit for external developers to use this ? Should they ever reach for it ? |
There was a problem hiding this comment.
What do you think about this ? Would we ever ask people to use this in a custom flow ?
There was a problem hiding this comment.
Most likely not, it should really be something that's handled internally.
| }; | ||
| #broadcastSignOutEvent = () => { | ||
| public __internal_broadcastSignOutEvent = () => { |
There was a problem hiding this comment.
Alternatively we could use the event bus
There was a problem hiding this comment.
👍 let's use the event bus, that way we can avoid a new Clerk method.
| }; | ||
| #broadcastSignOutEvent = () => { | ||
| public __internal_broadcastSignOutEvent = () => { |
There was a problem hiding this comment.
👍 let's use the event bus, that way we can avoid a new Clerk method.
| }); | ||
| }; | ||
| // TODO: Deprecate this one, and mark it as internal. Is there actual benefit for external developers to use this ? Should they ever reach for it ? |
There was a problem hiding this comment.
Most likely not, it should really be something that's handled internally.
| path: this.path() + '/sessions', | ||
| }) as unknown as Promise<ClientResource>; | ||
| }).then(e => { | ||
| SessionTokenCache.clear(); |
There was a problem hiding this comment.
I don't think this is the right place for this call. What's the desired behavior? SessionTokenCache is cleared on sign out?
There was a problem hiding this comment.
Correct, clearing the cache on sign out.
We're calling SessionTokenCache.clear() on Client.destroy(), on Session.end(), and on Session.remove(). Giving the fact that we clear the cache on an individual session removal, i think we should do the same when removing all of them.
Uh oh!
There was an error while loading. Please reload this page.
| // Notify other tabs that user is signing out. | ||
| eventBus.dispatch(events.UserSignOut, null); |
There was a problem hiding this comment.
❓ should we be doing this later in the sign out flow?
There was a problem hiding this comment.
Previously inside setActive it was one of the first things that got called. And since by that point we have already cleared cookies seems appropriate.
| export const events = { | ||
| TokenUpdate: 'token:update', | ||
| UserSignOut: 'user:signOut', |
There was a problem hiding this comment.
💭 (optional) what do you think about just calling this even signOut? I'm not sure we need the user: scope.
There was a problem hiding this comment.
I was thinking the same, I added the scope to respect the pattern. Since it does not do any harm, I think I'll leave it as is.
There was a problem hiding this comment.
Fair enough, I wouldn't say 1 event indicates a pattern though 😉
Description
Seems like our broadcasting logic inside
setActivereally depended on having the client being deleted and so thatupdateClientwould never be called to update Clerk.user and Clerk.session.Since Long lived clients, we are no longer deleting the client, which means
updateClientwould run beforesetActivecausing the broadcasting logic to always be skipped.The solution the PR suggests is to explicitly broadcast when
signOut()is called. As in improvement, we should broadcast the sign out event whenuser.delete()gets called.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change