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.

Sign up for GitHub

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

Jump to bottom

feat: don't emit SIGNED_IN events on visibility change #684

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+39 −32
Open

feat: don't emit SIGNED_IN events on visibility change #684

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
71 changes: 39 additions & 32 deletions src/GoTrueClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,7 @@ export default class GoTrueClient {
}

// no login attempt via callback url try to recover session from storage
await this._recoverAndRefresh()
await this._recoverAndRefresh(true)
return { error: null }
} catch (error) {
if (isAuthError(error)) {
Expand Down Expand Up @@ -660,10 +660,6 @@ export default class GoTrueClient {
}
}

/**
* Returns the session, refreshing it if necessary.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah is this section no longer needed/are we making getSession internal? Not sure if removing the comments will remove the description from reference spec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I'll investigate.

* The session returned can be null if the session is not detected which can happen in the event a user is not signed-in or has logged out.
*/
async getSession(): Promise<
| {
data: {
Expand All @@ -688,6 +684,32 @@ export default class GoTrueClient {
// save to just await, as long we make sure _initialize() never throws
await this.initializePromise

return await this._getSession()
}

/**
* Private version of {@link #getSession()} safe to reuse in private methods.
*/
private async _getSession(): Promise<
| {
data: {
session: Session
}
error: null
}
| {
data: {
session: null
}
error: AuthError
}
| {
data: {
session: null
}
error: null
}
> {
let currentSession: Session | null = null

if (this.persistSession) {
Expand All @@ -709,7 +731,7 @@ export default class GoTrueClient {
}

const hasExpired = currentSession.expires_at
? currentSession.expires_at <= Date.now() / 1000
? currentSession.expires_at - EXPIRY_MARGIN <= Date.now() / 1000
Copy link
Member

Choose a reason for hiding this comment

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

hmm i rmb we didn't do this before because there were some other places where we used getSession and checked the expiry margin too which would result in double counting - but just need to check again to be sure

Copy link
Contributor

Choose a reason for hiding this comment

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

Might have been https://github.com/supabase/gotrue-js/pull/684/files#diff-3522461172efd6058d6b8da62fc2d30d8b524d2b64894ea2c67218c52f7fdff5L1210 - definitely worth double checking

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right there was an issue... I'll actually add the Date.now() in the param so when called from _recoverAndRefresh it will use the current time + margin, and otherwise just the current time.

: false
if (!hasExpired) {
return { data: { session: currentSession }, error: null }
Expand Down Expand Up @@ -930,9 +952,9 @@ export default class GoTrueClient {
const { data, error } = await this.exchangeCodeForSession(authCode)
if (error) throw error
if (!data.session) throw new AuthPKCEGrantCodeExchangeError('No session detected.')
let url = new URL(window.location.href);
let url = new URL(window.location.href)
url.searchParams.delete('code')
window.history.replaceState(window.history.state, "", url.toString())
window.history.replaceState(window.history.state, '', url.toString())
return { data: { session: data.session, redirectType: null }, error: null }
}

Expand Down Expand Up @@ -1194,32 +1216,17 @@ export default class GoTrueClient {
* Recovers the session from LocalStorage and refreshes
* Note: this method is async to accommodate for AsyncStorage e.g. in React native.
*/
private async _recoverAndRefresh() {
private async _recoverAndRefresh(isInitial: boolean) {
Copy link
Member

Choose a reason for hiding this comment

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

what does the isInitial param do here? can we document it above?

try {
const currentSession = await getItemAsync(this.storage, this.storageKey)
if (!this._isValidSession(currentSession)) {
if (currentSession !== null) {
await this._removeSession()
}

return
const {
data: { session: currentSession },
error,
} = await this._getSession()
if (error) {
throw error
}

const timeNow = Math.round(Date.now() / 1000)

if ((currentSession.expires_at ?? Infinity) < timeNow + EXPIRY_MARGIN) {
if (this.autoRefreshToken && currentSession.refresh_token) {
const { error } = await this._callRefreshToken(currentSession.refresh_token)

if (error) {
console.log(error.message)
await this._removeSession()
}
}
} else {
if (this.persistSession) {
await this._saveSession(currentSession)
}
Comment on lines -1208 to -1222
Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need to have 2 different versions that do refreshing when getSession does it all. It uses _getSession because _recoverAndRefresh is part of the _initialize flow and it means that getSession would never complete.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah okay, so to expand - if we called getSession instead of _getSession() this would fail as getSession would call intializePromise which would call _initialize() which would then call _recoverAndRefresh leading to an infinite loop?

if (isInitial && currentSession) {
this._notifyAllSubscribers('SIGNED_IN', currentSession)
}
} catch (err) {
Expand Down Expand Up @@ -1468,7 +1475,7 @@ export default class GoTrueClient {
if (!isInitial) {
// initial visibility change setup is handled in another flow under #initialize()
await this.initializePromise
await this._recoverAndRefresh()
await this._recoverAndRefresh(false)
}

if (this.autoRefreshToken) {
Expand Down