-
Notifications
You must be signed in to change notification settings - Fork 402
fix(clerk-js): Broadcast sign out to all tabs on long lived clients #5133
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
Changes from all commits
d28680c
4e926cd
16aa646
e8c1e29
8c98956
8e613d3
b71c79b
3539358
0bcd8b1
3cd011e
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-js': patch | ||
| --- | ||
|
|
||
| Bug fix: Broadcast a sign out event to all opened tabs when `Clerk.signOut()` or `User.delete()` is called. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -374,6 +374,9 @@ export class Clerk implements ClerkInterface { | |
|
|
||
| const handleSetActive = () => { | ||
| const signOutCallback = typeof callbackOrOptions === 'function' ? callbackOrOptions : undefined; | ||
|
|
||
| // Notify other tabs that user is signing out. | ||
| eventBus.dispatch(events.UserSignOut, null); | ||
|
Comment on lines
+378
to
+379
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. ❓ should we be doing this later in the sign out flow?
Member
Author
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. 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. |
||
| if (signOutCallback) { | ||
| return this.setActive({ | ||
| session: null, | ||
|
|
@@ -908,14 +911,6 @@ export class Clerk implements ClerkInterface { | |
|
|
||
| await onBeforeSetActive(); | ||
|
|
||
| // 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 }); | ||
| } | ||
|
|
||
|
Comment on lines
-911
to
-918
Member
Author
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. Since we are no longer delete the client on sign out, Also removing |
||
| //1. setLastActiveSession to passed user session (add a param). | ||
| // Note that this will also update the session's active organization | ||
| // id. | ||
|
|
@@ -1534,6 +1529,7 @@ export class Clerk implements ClerkInterface { | |
| }); | ||
| }; | ||
|
|
||
| // 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 ? | ||
|
Member
Author
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. What do you think about this ? Would we ever ask people to use this in a custom flow ?
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. Most likely not, it should really be something that's handled internally. |
||
| public handleUnauthenticated = async (opts = { broadcast: true }): Promise<unknown> => { | ||
| if (!this.client || !this.session) { | ||
| return; | ||
|
|
@@ -1545,7 +1541,7 @@ export class Clerk implements ClerkInterface { | |
| return; | ||
| } | ||
| if (opts.broadcast) { | ||
| this.#broadcastSignOutEvent(); | ||
| eventBus.dispatch(events.UserSignOut, null); | ||
| } | ||
| return this.setActive({ session: null }); | ||
| } catch (err) { | ||
|
|
@@ -2061,11 +2057,21 @@ export class Clerk implements ClerkInterface { | |
| this.#sessionTouchOfflineScheduler.schedule(performTouch); | ||
| }); | ||
|
|
||
| /** | ||
| * Background tabs get notified of a signout event from active tab. | ||
| */ | ||
| this.#broadcastChannel?.addEventListener('message', ({ data }) => { | ||
| if (data.type === 'signout') { | ||
| void this.handleUnauthenticated(); | ||
| void this.handleUnauthenticated({ broadcast: false }); | ||
| } | ||
| }); | ||
|
|
||
| /** | ||
| * Allow resources within the singleton to notify other tabs about a signout event (scoped to a single tab) | ||
| */ | ||
| eventBus.on(events.UserSignOut, () => { | ||
| this.#broadcastChannel?.postMessage({ type: 'signout' }); | ||
| }); | ||
| }; | ||
|
|
||
| // TODO: Be more conservative about touches. Throttle, don't touch when only one user, etc | ||
|
|
@@ -2100,10 +2106,6 @@ export class Clerk implements ClerkInterface { | |
| } | ||
| }; | ||
|
|
||
| #broadcastSignOutEvent = () => { | ||
| this.#broadcastChannel?.postMessage({ type: 'signout' }); | ||
| }; | ||
|
|
||
| #setTransitiveState = () => { | ||
| this.session = undefined; | ||
| this.organization = undefined; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ import type { TokenResource } from '@clerk/types'; | |
|
|
||
| export const events = { | ||
| TokenUpdate: 'token:update', | ||
| UserSignOut: 'user:signOut', | ||
|
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. 💭 (optional) what do you think about just calling this even
Member
Author
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 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.
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. Fair enough, I wouldn't say 1 event indicates a pattern though 😉 |
||
| } as const; | ||
|
|
||
| type ClerkEvent = (typeof events)[keyof typeof events]; | ||
|
|
@@ -11,6 +12,7 @@ type TokenUpdatePayload = { token: TokenResource | null }; | |
|
|
||
| type EventPayload = { | ||
| [events.TokenUpdate]: TokenUpdatePayload; | ||
| [events.UserSignOut]: null; | ||
| }; | ||
|
|
||
| const createEventBus = () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -83,7 +83,10 @@ export class Client extends BaseResource implements ClientResource { | |
| removeSessions(): Promise<ClientResource> { | ||
| return this._baseDelete({ | ||
| path: this.path() + '/sessions', | ||
| }) as unknown as Promise<ClientResource>; | ||
| }).then(e => { | ||
| SessionTokenCache.clear(); | ||
|
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. I don't think this is the right place for this call. What's the desired behavior?
Member
Author
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. Correct, clearing the cache on sign out. We're calling |
||
| return e as unknown as ClientResource; | ||
| }); | ||
| } | ||
|
|
||
| clearCache(): void { | ||
|
|
||
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.
Previously the test was passing after 40-50 seconds because either /touch or /tokens would force update the state.