-
-
Notifications
You must be signed in to change notification settings - Fork 171
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: resolve & reset deferred upon refresh error #339
Changes from 2 commits
90ed495
ff228a7
d955804
1fead6b
176d15c
7e20b31
709e3f4
828b0b3
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 |
---|---|---|
|
@@ -37,6 +37,14 @@ import type { | |
|
||
polyfillGlobalThis() // Make "globalThis" available | ||
|
||
type CallRefreshTokenResult = { | ||
session: Session | ||
error: null | ||
} | { | ||
session: null | ||
error: AuthError | ||
} | ||
|
||
const DEFAULT_OPTIONS = { | ||
url: GOTRUE_URL, | ||
storageKey: STORAGE_KEY, | ||
|
@@ -83,10 +91,7 @@ export default class GoTrueClient { | |
protected refreshTokenTimer?: ReturnType<typeof setTimeout> | ||
// eslint-disable-next-line @typescript-eslint/no-inferrable-types | ||
protected networkRetries: number = 0 | ||
protected refreshingDeferred: Deferred<{ | ||
session: Session | ||
error: null | ||
}> | null = null | ||
protected refreshingDeferred: Deferred<CallRefreshTokenResult> | null = null | ||
|
||
/** | ||
* Create a new client for use in the browser. | ||
|
@@ -884,17 +889,16 @@ export default class GoTrueClient { | |
} | ||
} | ||
|
||
private async _callRefreshToken(refresh_token = this.currentSession?.refresh_token) { | ||
try { | ||
// refreshing is already in progress | ||
if (this.refreshingDeferred) { | ||
return await this.refreshingDeferred.promise | ||
} | ||
private async _callRefreshToken( | ||
refresh_token = this.currentSession?.refresh_token | ||
): Promise<CallRefreshTokenResult> { | ||
// refreshing is already in progress | ||
if (this.refreshingDeferred) { | ||
return this.refreshingDeferred.promise | ||
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. Moved that out of the try block and removed the |
||
} | ||
|
||
this.refreshingDeferred = new Deferred<{ | ||
session: Session | ||
error: null | ||
}>() | ||
try { | ||
this.refreshingDeferred = new Deferred<CallRefreshTokenResult>() | ||
|
||
if (!refresh_token) { | ||
throw new Error('No current session.') | ||
|
@@ -910,15 +914,21 @@ export default class GoTrueClient { | |
const result = { session, error: null } | ||
|
||
this.refreshingDeferred.resolve(result) | ||
this.refreshingDeferred = null | ||
|
||
return result | ||
} catch (error) { | ||
if (isAuthError(error)) { | ||
return { session: null, error } | ||
const result = { session: null, error }; | ||
|
||
this.refreshingDeferred?.resolve(result); | ||
|
||
return result; | ||
} | ||
|
||
this.refreshingDeferred?.reject(error); | ||
throw error | ||
} finally { | ||
this.refreshingDeferred = null | ||
} | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
import { OpenIDConnectCredentials } from '../src' | ||
import { AuthError } from '../src/lib/errors'; | ||
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. Errors are not exported by the library. Would it make sense to export them so a consumer can import errors and compare them with 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. Good point – we definitely should at least be exporting |
||
import { | ||
authClient as auth, | ||
authClientWithSession as authWithSession, | ||
|
@@ -133,6 +134,71 @@ describe('GoTrueClient', () => { | |
expect(refreshAccessTokenSpy).toBeCalledTimes(1) | ||
}) | ||
|
||
test('refreshSession() should resolve all pending refresh requests and reset deferred upon AuthError', async () => { | ||
const { email, password } = mockUserCredentials() | ||
refreshAccessTokenSpy.mockImplementationOnce(() => Promise.resolve({ | ||
session: null, | ||
error: new AuthError('Something did not work as expected') | ||
})) | ||
|
||
const { error, session } = await authWithSession.signUp({ | ||
email, | ||
password, | ||
}) | ||
|
||
expect(error).toBeNull() | ||
expect(session).not.toBeNull() | ||
|
||
const [{ session: session1, error: error1 }, { session: session2, error: error2 }] = | ||
await Promise.all([authWithSession.refreshSession(), authWithSession.refreshSession()]) | ||
|
||
expect(error1).toHaveProperty('message') | ||
expect(error2).toHaveProperty('message') | ||
expect(session1).toBeNull() | ||
expect(session2).toBeNull() | ||
|
||
expect(refreshAccessTokenSpy).toBeCalledTimes(1) | ||
|
||
// vreify the deferred has been reset and successive calls can be made | ||
const { session: session3, error: error3 } = await authWithSession.refreshSession() | ||
|
||
expect(error3).toBeNull() | ||
expect(session3).toHaveProperty('access_token') | ||
}) | ||
|
||
test('refreshSession() should reject all pending refresh requests and reset deferred upon any non AuthError', async () => { | ||
const mockError = new Error('Something did not work as expected'); | ||
|
||
const { email, password } = mockUserCredentials() | ||
refreshAccessTokenSpy.mockImplementationOnce(() => Promise.reject(mockError)) | ||
|
||
const { error, session } = await authWithSession.signUp({ | ||
email, | ||
password, | ||
}) | ||
|
||
expect(error).toBeNull() | ||
expect(session).not.toBeNull() | ||
|
||
const [error1, error2] = | ||
await Promise.allSettled([authWithSession.refreshSession(), authWithSession.refreshSession()]) | ||
|
||
expect(error1.status).toEqual('rejected') | ||
expect(error2.status).toEqual('rejected') | ||
|
||
// status === 'rejected' above makes sure it is a PromiseRejectedResult | ||
expect((error1 as PromiseRejectedResult).reason).toEqual(mockError) | ||
expect((error1 as PromiseRejectedResult).reason).toEqual(mockError) | ||
|
||
expect(refreshAccessTokenSpy).toBeCalledTimes(1) | ||
|
||
// vreify the deferred has been reset and successive calls can be made | ||
const { session: session3, error: error3 } = await authWithSession.refreshSession() | ||
|
||
expect(error3).toBeNull() | ||
expect(session3).toHaveProperty('access_token') | ||
}) | ||
|
||
test('getSessionFromUrl() can only be called from a browser', async () => { | ||
const { error, session } = await authWithSession.getSessionFromUrl() | ||
|
||
|
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.
This type is solely used in private methods and properties. Not sure if it nevertheless should go to
lib/types.ts
? Both is fine for me.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.
Yeah let's put this in
lib/types.ts
to keep things consistent 😄