-
Notifications
You must be signed in to change notification settings - Fork 469
fix(react): finalize ticket flows started before Clerk loads #9628
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
base: main
Are you sure you want to change the base?
Changes from all commits
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/react': patch | ||
| --- | ||
|
|
||
| Ensure ticket-based sign-in and sign-up flows started before Clerk finishes loading can be finalized successfully. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,167 @@ | ||
| import type { SignInFutureResource, SignUpFutureResource } from '@clerk/shared/types'; | ||
| import { describe, expect, it, vi } from 'vitest'; | ||
|
|
||
| import { StateProxy } from '../stateProxy'; | ||
|
|
||
| describe('StateProxy', () => { | ||
| it('preserves a completed sign-in across chained calls when the client clears its sign-in attempt', async () => { | ||
| const emptySignIn = { | ||
| status: 'needs_identifier', | ||
| createdSessionId: null as string | null, | ||
| ticket: vi.fn(() => Promise.resolve({ error: null })), | ||
| finalize: vi.fn(() => Promise.reject(new Error('Cannot finalize sign-in without a created session.'))), | ||
| }; | ||
| const completedSignIn = { | ||
| status: 'needs_identifier', | ||
| createdSessionId: null as string | null, | ||
| ticket: vi.fn(() => { | ||
| client.signIn = { __internal_future: emptySignIn }; | ||
| completedSignIn.status = 'complete'; | ||
| completedSignIn.createdSessionId = 'sess_123'; | ||
| return Promise.resolve({ error: null }); | ||
| }), | ||
| finalize: vi.fn(() => Promise.resolve({ error: null })), | ||
| }; | ||
| const client = { | ||
| signIn: { __internal_future: completedSignIn }, | ||
| }; | ||
| const state = { | ||
| signInSignal: () => ({ signIn: completedSignIn }), | ||
| }; | ||
| const loadedCallbacks: Array<() => void> = []; | ||
| const isomorphicClerk = { | ||
| loaded: false, | ||
| client, | ||
| __internal_state: state, | ||
| addOnLoaded: vi.fn((callback: () => void) => loadedCallbacks.push(callback)), | ||
| }; | ||
| const signIn = new StateProxy(isomorphicClerk as any).signInSignal().signIn as SignInFutureResource; | ||
|
|
||
| const ticketPromise = signIn.ticket({ ticket: 'ticket_123' }); | ||
| expect(isomorphicClerk.addOnLoaded).toHaveBeenCalledOnce(); | ||
| expect(completedSignIn.ticket).not.toHaveBeenCalled(); | ||
|
|
||
| isomorphicClerk.loaded = true; | ||
| loadedCallbacks.forEach(callback => callback()); | ||
| await ticketPromise; | ||
|
|
||
| await expect(signIn.finalize()).resolves.toEqual({ error: null }); | ||
| expect(signIn.status).toBe('complete'); | ||
| expect(signIn.createdSessionId).toBe('sess_123'); | ||
| expect(completedSignIn.finalize).toHaveBeenCalledOnce(); | ||
| expect(emptySignIn.finalize).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('preserves a completed sign-up across chained calls when the client clears its sign-up attempt', async () => { | ||
| const emptySignUp = { | ||
| status: 'missing_requirements', | ||
| createdSessionId: null as string | null, | ||
| ticket: vi.fn(() => Promise.resolve({ error: null })), | ||
| finalize: vi.fn(() => | ||
| Promise.resolve({ error: new Error('Cannot finalize sign-up without a created session.') }), | ||
| ), | ||
| }; | ||
| const completedSignUp = { | ||
| status: 'missing_requirements', | ||
| createdSessionId: null as string | null, | ||
| ticket: vi.fn(() => { | ||
| client.signUp = { __internal_future: emptySignUp }; | ||
| completedSignUp.status = 'complete'; | ||
| completedSignUp.createdSessionId = 'sess_123'; | ||
| return Promise.resolve({ error: null }); | ||
| }), | ||
| finalize: vi.fn(() => Promise.resolve({ error: null })), | ||
| }; | ||
| const client: { | ||
| signUp: { __internal_future: typeof completedSignUp | typeof emptySignUp }; | ||
| } = { | ||
| signUp: { __internal_future: completedSignUp }, | ||
| }; | ||
| const state = { | ||
| signUpSignal: () => ({ signUp: completedSignUp }), | ||
| }; | ||
| const loadedCallbacks: Array<() => void> = []; | ||
| const isomorphicClerk = { | ||
| loaded: false, | ||
| client, | ||
| __internal_state: state, | ||
| addOnLoaded: vi.fn((callback: () => void) => loadedCallbacks.push(callback)), | ||
| }; | ||
| const signUp = new StateProxy(isomorphicClerk as any).signUpSignal().signUp as SignUpFutureResource; | ||
|
|
||
| const ticketPromise = signUp.ticket({ ticket: 'ticket_123' }); | ||
| expect(isomorphicClerk.addOnLoaded).toHaveBeenCalledOnce(); | ||
| expect(completedSignUp.ticket).not.toHaveBeenCalled(); | ||
|
|
||
| isomorphicClerk.loaded = true; | ||
| loadedCallbacks.forEach(callback => callback()); | ||
| await ticketPromise; | ||
|
|
||
| await expect(signUp.finalize()).resolves.toEqual({ error: null }); | ||
| expect(signUp.status).toBe('complete'); | ||
| expect(signUp.createdSessionId).toBe('sess_123'); | ||
| expect(completedSignUp.finalize).toHaveBeenCalledOnce(); | ||
| expect(emptySignUp.finalize).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('falls back to the client sign-in when the state signal is empty', async () => { | ||
| const clientSignIn = { | ||
| status: 'needs_first_factor', | ||
| create: vi.fn(() => Promise.resolve({ error: null })), | ||
| }; | ||
| const isomorphicClerk = { | ||
| loaded: true, | ||
| client: { signIn: { __internal_future: clientSignIn } }, | ||
| __internal_state: { signInSignal: () => ({ signIn: null }) }, | ||
| }; | ||
| const signIn = new StateProxy(isomorphicClerk as any).signInSignal().signIn as SignInFutureResource; | ||
|
|
||
| await expect(signIn.create({ identifier: 'test@example.com' })).resolves.toEqual({ error: null }); | ||
| expect(signIn.status).toBe('needs_first_factor'); | ||
| expect(clientSignIn.create).toHaveBeenCalledOnce(); | ||
| }); | ||
|
|
||
| it('uses a newer sign-in from the state signal instead of the client attempt', async () => { | ||
| const clientSignIn = { | ||
| finalize: vi.fn(() => Promise.reject(new Error('Finalized the stale client sign-in.'))), | ||
| }; | ||
| const currentSignIn = { | ||
| finalize: vi.fn(() => Promise.resolve({ error: null })), | ||
| }; | ||
| const isomorphicClerk = { | ||
| loaded: true, | ||
| client: { signIn: { __internal_future: clientSignIn } }, | ||
| __internal_state: { signInSignal: () => ({ signIn: currentSignIn }) }, | ||
| }; | ||
| const signIn = new StateProxy(isomorphicClerk as any).signInSignal().signIn as SignInFutureResource; | ||
|
|
||
| await expect(signIn.finalize()).resolves.toEqual({ error: null }); | ||
| expect(currentSignIn.finalize).toHaveBeenCalledOnce(); | ||
| expect(clientSignIn.finalize).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('falls back to the fresh client sign-in after the retained state attempt is cleared', async () => { | ||
| const clientSignIn = { | ||
| status: 'needs_identifier', | ||
| }; | ||
| let stateSignIn: { status: string; finalize: ReturnType<typeof vi.fn> } | null; | ||
| const completedSignIn = { | ||
| status: 'complete', | ||
| finalize: vi.fn(() => { | ||
| stateSignIn = null; | ||
| return Promise.resolve({ error: null }); | ||
| }), | ||
| }; | ||
| stateSignIn = completedSignIn; | ||
| const isomorphicClerk = { | ||
| loaded: true, | ||
| client: { signIn: { __internal_future: clientSignIn } }, | ||
| __internal_state: { signInSignal: () => ({ signIn: stateSignIn }) }, | ||
| }; | ||
| const signIn = new StateProxy(isomorphicClerk as any).signInSignal().signIn as SignInFutureResource; | ||
|
|
||
| expect(signIn.status).toBe('complete'); | ||
| await expect(signIn.finalize()).resolves.toEqual({ error: null }); | ||
| expect(signIn.status).toBe('needs_identifier'); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,13 +134,15 @@ export class StateProxy implements State { | |
|
|
||
| private buildSignInProxy() { | ||
| const gateProperty = this.gateProperty.bind(this); | ||
| const target = () => this.client.signIn.__internal_future; | ||
| const target = () => this.state.signInSignal().signIn ?? this.client.signIn.__internal_future; | ||
|
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. in what case would
Contributor
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. typically they match, but after a ticket completes, the client can replace signIn with an empty attempt while the state signal keeps the completed attempt until finalize() or reset(). the proxy was calling finalize() on the new empty client attempt instead of the retained one
Contributor
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. {
"response": {
"object": "sign_in_attempt",
"id": "sia_REDACTED",
"status": "complete",
"supported_identifiers": [
"email_address"
],
"supported_first_factors": null,
"supported_second_factors": null,
"first_factor_verification": {
"object": "verification_ticket",
"status": "verified",
"strategy": "ticket",
"attempts": null,
"expire_at": null
},
"second_factor_verification": null,
"identifier": "REDACTED",
"user_data": null,
"created_session_id": "sess_REDACTED",
"abandon_at": "REDACTED_TIMESTAMP",
"locale": "en-US"
},
"client": {
"object": "client",
"id": "client_REDACTED",
"sessions": [
{
"object": "session",
"id": "sess_REDACTED",
"status": "active",
"expire_at": "REDACTED_TIMESTAMP",
"abandon_at": "REDACTED_TIMESTAMP",
"last_active_at": "REDACTED_TIMESTAMP",
"last_active_organization_id": null,
"actor": null,
"user": "REDACTED_USER_OBJECT",
"public_user_data": "REDACTED_PUBLIC_USER_DATA",
"factor_verification_age": [
0,
-1
],
"created_at": "REDACTED_TIMESTAMP",
"updated_at": "REDACTED_TIMESTAMP",
"last_active_token": {
"object": "token",
"jwt": "REDACTED_JWT"
}
}
],
"sign_in": null,
"sign_up": null,
"last_active_session_id": "sess_REDACTED",
"last_authentication_strategy": "ticket",
"cookie_expires_at": null,
"captcha_bypass": false,
"created_at": "REDACTED_TIMESTAMP",
"updated_at": "REDACTED_TIMESTAMP"
}
} |
||
|
|
||
| return { | ||
| errors: defaultSignInErrors(), | ||
| fetchStatus: 'idle' as const, | ||
| signIn: { | ||
| status: 'needs_identifier' as const, | ||
| get status() { | ||
| return gateProperty(target, 'status', 'needs_identifier'); | ||
| }, | ||
|
Comment on lines
+143
to
+145
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. this makes sense to me! |
||
| availableStrategies: [], | ||
| get isTransferable() { | ||
| return gateProperty(target, 'isTransferable', false); | ||
|
|
@@ -255,7 +257,7 @@ export class StateProxy implements State { | |
| private buildSignUpProxy() { | ||
| const gateProperty = this.gateProperty.bind(this); | ||
| const gateMethod = this.gateMethod.bind(this); | ||
| const target = () => this.client.signUp.__internal_future; | ||
| const target = () => this.state.signUpSignal().signUp ?? this.client.signUp.__internal_future; | ||
|
|
||
| return { | ||
| errors: defaultSignUpErrors(), | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.