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-js): Remove setActive() call from signOut() #5171

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/dry-tigers-mate.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@clerk/clerk-js': patch
---

Fixes an issue where calling `signOut()` would not always redirect.
34 changes: 10 additions & 24 deletions packages/clerk-js/src/core/__tests__/clerk.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -587,16 +587,13 @@ describe('Clerk singleton', () => {
);

const sut = new Clerk(productionPublishableKey);
sut.setActive = jest.fn();
sut.navigate = jest.fn();
await sut.load();
await sut.signOut();
await waitFor(() => {
expect(mockClientDestroy).not.toHaveBeenCalled();
expect(mockClientRemoveSessions).toHaveBeenCalled();
expect(sut.setActive).toHaveBeenCalledWith({
session: null,
redirectUrl: '/',
});
expect(sut.navigate).toHaveBeenCalledWith('/');
});
});

Expand All @@ -613,17 +610,14 @@ describe('Clerk singleton', () => {
);

const sut = new Clerk(productionPublishableKey);
sut.setActive = jest.fn();
sut.navigate = jest.fn();
await sut.load();
await sut.signOut();
await waitFor(() => {
expect(mockClientDestroy).not.toHaveBeenCalled();
expect(mockClientRemoveSessions).toHaveBeenCalled();
expect(mockSession1.remove).not.toHaveBeenCalled();
expect(sut.setActive).toHaveBeenCalledWith({
session: null,
redirectUrl: '/',
});
expect(sut.navigate).toHaveBeenCalledWith('/');
});
},
);
Expand All @@ -638,15 +632,13 @@ describe('Clerk singleton', () => {
);

const sut = new Clerk(productionPublishableKey);
sut.setActive = jest.fn();
sut.navigate = jest.fn();
await sut.load();
await sut.signOut({ sessionId: '2' });
await waitFor(() => {
expect(mockSession2.remove).toHaveBeenCalled();
expect(mockClientDestroy).not.toHaveBeenCalled();
expect(sut.setActive).not.toHaveBeenCalledWith({
session: null,
});
expect(sut.navigate).not.toHaveBeenCalled();
});
});

Expand All @@ -660,16 +652,13 @@ describe('Clerk singleton', () => {
);

const sut = new Clerk(productionPublishableKey);
sut.setActive = jest.fn();
sut.navigate = jest.fn();
await sut.load();
await sut.signOut({ sessionId: '1' });
await waitFor(() => {
expect(mockSession1.remove).toHaveBeenCalled();
expect(mockClientDestroy).not.toHaveBeenCalled();
expect(sut.setActive).toHaveBeenCalledWith({
session: null,
redirectUrl: '/',
});
expect(sut.navigate).toHaveBeenCalledWith('/');
});
});

Expand All @@ -683,16 +672,13 @@ describe('Clerk singleton', () => {
);

const sut = new Clerk(productionPublishableKey);
sut.setActive = jest.fn();
sut.navigate = jest.fn();
await sut.load();
await sut.signOut({ sessionId: '1', redirectUrl: '/after-sign-out' });
await waitFor(() => {
expect(mockSession1.remove).toHaveBeenCalled();
expect(mockClientDestroy).not.toHaveBeenCalled();
expect(sut.setActive).toHaveBeenCalledWith({
session: null,
redirectUrl: '/after-sign-out',
});
expect(sut.navigate).toHaveBeenCalledWith('/after-sign-out');
});
});
});
Expand Down
118 changes: 77 additions & 41 deletions packages/clerk-js/src/core/clerk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,6 @@ import {
getClerkQueryParam,
getWeb3Identifier,
hasExternalAccountSignUpError,
ignoreEventValue,
inActiveBrowserTab,
inBrowser,
isDevAccountPortalOrigin,
Expand Down Expand Up @@ -128,6 +127,8 @@ import {
} from './resources/internal';
import { warnings } from './warnings';

type SetActiveHook = (intent?: 'sign-out') => void | Promise<void>;

export type ClerkCoreBroadcastChannelEvent = { type: 'signout' };

declare global {
Expand Down Expand Up @@ -372,43 +373,79 @@ export class Clerk implements ClerkInterface {
if (!this.client || this.client.sessions.length === 0) {
return;
}

const onBeforeSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onBeforeSetActive === 'function'
? window.__unstable__onBeforeSetActive
: noop;

const onAfterSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onAfterSetActive === 'function'
? window.__unstable__onAfterSetActive
: noop;

const opts = callbackOrOptions && typeof callbackOrOptions === 'object' ? callbackOrOptions : options || {};

const redirectUrl = opts?.redirectUrl || this.buildAfterSignOutUrl();
const signOutCallback = typeof callbackOrOptions === 'function' ? callbackOrOptions : undefined;

const handleSetActive = () => {
const signOutCallback = typeof callbackOrOptions === 'function' ? callbackOrOptions : undefined;
const executeSignOut = async () => {
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser);

/**
* Hint to each framework, that the user will be signed out.
*/
await onBeforeSetActive('sign-out');
// Notify other tabs that user is signing out.
eventBus.dispatch(events.UserSignOut, null);
if (signOutCallback) {
return this.setActive({
session: null,
beforeEmit: ignoreEventValue(signOutCallback),
});
}
// Clean up cookies
eventBus.dispatch(events.TokenUpdate, { token: null });

return this.setActive({
session: null,
redirectUrl,
this.#setTransitiveState();

await tracker.track(async () => {
if (signOutCallback) {
await signOutCallback();
} else {
await this.navigate(redirectUrl);
}
});

if (tracker.isUnloading()) {
return;
}

this.#setAccessors();
this.#emit();

await onAfterSetActive();
};

/**
* Clears the router cache for `@clerk/nextjs` on all routes except the current one.
* Note: Calling `onBeforeSetActive` before signing out, allows for new RSC prefetch requests to render as signed in.
*/
await onBeforeSetActive();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Notice that we call onBeforeSetActive twice

if (!opts.sessionId || this.client.signedInSessions.length === 1) {
if (this.#options.experimental?.persistClient ?? true) {
await this.client.removeSessions();
} else {
await this.client.destroy();
}

return handleSetActive();
await executeSignOut();

return;
}

// Multi-session handling
const session = this.client.signedInSessions.find(s => s.id === opts.sessionId);
const shouldSignOutCurrent = session?.id && this.session?.id === session.id;

await session?.remove();

if (shouldSignOutCurrent) {
return handleSetActive();
await executeSignOut();
}
};

Expand Down Expand Up @@ -872,7 +909,6 @@ export class Clerk implements ClerkInterface {
);
}

type SetActiveHook = () => void | Promise<void>;
const onBeforeSetActive: SetActiveHook =
typeof window !== 'undefined' && typeof window.__unstable__onBeforeSetActive === 'function'
? window.__unstable__onBeforeSetActive
Expand Down Expand Up @@ -913,7 +949,10 @@ export class Clerk implements ClerkInterface {
eventBus.dispatch(events.TokenUpdate, { token: session.lastActiveToken });
}

await onBeforeSetActive();
/**
* Hint to each framework, that the user will be signed out when `{session: null}` is provided.
*/
await onBeforeSetActive(newSession === null ? 'sign-out' : undefined);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to support the case of User.delete and the setActive({session:null})


//1. setLastActiveSession to passed user session (add a param).
// Note that this will also update the session's active organization
Expand All @@ -934,40 +973,41 @@ export class Clerk implements ClerkInterface {
// undefined, then wait for beforeEmit to complete before emitting the new session.
// When undefined, neither SignedIn nor SignedOut renders, which avoids flickers or
// automatic reloading when reloading shouldn't be happening.
const beforeUnloadTracker = this.#options.standardBrowser ? createBeforeUnloadTracker() : undefined;
const tracker = createBeforeUnloadTracker(this.#options.standardBrowser);

if (beforeEmit) {
deprecated(
'Clerk.setActive({beforeEmit})',
'Use the `redirectUrl` property instead. Example `Clerk.setActive({redirectUrl:"/"})`',
);
beforeUnloadTracker?.startTracking();
this.#setTransitiveState();
await beforeEmit(newSession);
beforeUnloadTracker?.stopTracking();
await tracker.track(async () => {
this.#setTransitiveState();
await beforeEmit(newSession);
});
}

if (redirectUrl && !beforeEmit) {
beforeUnloadTracker?.startTracking();
this.#setTransitiveState();

if (this.client.isEligibleForTouch()) {
const absoluteRedirectUrl = new URL(redirectUrl, window.location.href);

await this.navigate(this.buildUrlWithAuth(this.client.buildTouchUrl({ redirectUrl: absoluteRedirectUrl })));
} else {
await this.navigate(redirectUrl);
}

beforeUnloadTracker?.stopTracking();
await tracker.track(async () => {
if (!this.client) {
// Typescript is not happy because since thinks this.client might have changed to undefined because the function is asynchronous.
return;
}
this.#setTransitiveState();
if (this.client.isEligibleForTouch()) {
const absoluteRedirectUrl = new URL(redirectUrl, window.location.href);
await this.navigate(this.buildUrlWithAuth(this.client.buildTouchUrl({ redirectUrl: absoluteRedirectUrl })));
} else {
await this.navigate(redirectUrl);
}
});
}

//3. Check if hard reloading (onbeforeunload). If not, set the user/session and emit
if (beforeUnloadTracker?.isUnloading()) {
//3. Check if hard reloading (onbeforeunload). If not, set the user/session and emit
if (tracker.isUnloading()) {
return;
}

this.#setAccessors(newSession);

this.#emit();
await onAfterSetActive();
};
Expand Down Expand Up @@ -2127,17 +2167,13 @@ export class Clerk implements ClerkInterface {
#setAccessors = (session?: SignedInSessionResource | null) => {
this.session = session || null;
this.organization = this.#getLastActiveOrganizationFromSession();
this.#aliasUser();
this.user = this.session ? this.session.user : null;
};

#getSessionFromClient = (sessionId: string | undefined): SignedInSessionResource | null => {
return this.client?.signedInSessions.find(x => x.id === sessionId) || null;
};

#aliasUser = () => {
this.user = this.session ? this.session.user : null;
};

#handleImpersonationFab = () => {
this.addListener(({ session }) => {
const isImpersonating = !!session?.actor;
Expand Down
2 changes: 1 addition & 1 deletion packages/clerk-js/src/global.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,6 @@ declare const __DEV__: boolean;
declare const __BUILD_DISABLE_RHC__: string;

interface Window {
__unstable__onBeforeSetActive: () => Promise<void> | void;
__unstable__onBeforeSetActive: (intent?: 'sign-out') => Promise<void> | void;
__unstable__onAfterSetActive: () => Promise<void> | void;
}
29 changes: 25 additions & 4 deletions packages/clerk-js/src/utils/beforeUnloadTracker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,22 +11,43 @@ import { CLERK_BEFORE_UNLOAD_EVENT } from './windowNavigate';
*
* @internal
*/
export const createBeforeUnloadTracker = () => {
const createBeforeUnloadListener = () => {
let _isUnloading = false;

const toggle = () => (_isUnloading = true);

const startTracking = () => {
const startListening = () => {
window.addEventListener('beforeunload', toggle);
window.addEventListener(CLERK_BEFORE_UNLOAD_EVENT, toggle);
};

const stopTracking = () => {
const stopListening = () => {
window.removeEventListener('beforeunload', toggle);
window.removeEventListener(CLERK_BEFORE_UNLOAD_EVENT, toggle);
};

const isUnloading = () => _isUnloading;

return { startTracking, stopTracking, isUnloading };
return { startListening, stopListening, isUnloading };
};

export const createBeforeUnloadTracker = (enabled = false) => {
if (!enabled) {
return {
track: async (fn: () => Promise<void>) => {
await fn();
},
isUnloading: () => false,
};
}

const l = createBeforeUnloadListener();
return {
track: async (fn: () => Promise<void>) => {
l.startListening();
await fn();
l.stopListening();
},
isUnloading: l.isUnloading,
};
};
8 changes: 6 additions & 2 deletions packages/nextjs/src/app-router/client/ClerkProvider.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ const NextClientClerkProvider = (props: NextClerkProviderProps) => {
}, [isPending]);

useSafeLayoutEffect(() => {
window.__unstable__onBeforeSetActive = () => {
window.__unstable__onBeforeSetActive = intent => {
/**
* We need to invalidate the cache in case the user is navigating to a page that
* was previously cached using the auth state that was active at the time.
Expand All @@ -78,11 +78,15 @@ const NextClientClerkProvider = (props: NextClerkProviderProps) => {
return new Promise(res => {
window.__clerk_internal_invalidateCachePromise = res;

const nextVersion = window?.next?.version || '';

// NOTE: the following code will allow `useReverification()` to work properly when `handlerReverification` is called inside `startTransition`
if (window.next?.version && typeof window.next.version === 'string' && window.next.version.startsWith('13')) {
if (nextVersion.startsWith('13')) {
startTransition(() => {
router.refresh();
});
} else if (nextVersion.startsWith('15') && intent === 'sign-out') {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On next 15 we still want to fire the server action when setting a session or when selecting an organization.

res(); // noop
} else {
void invalidateCacheAction().then(() => res());
}
Expand Down
Loading
Loading