feat: don't emit SIGNED_IN events on visibility change
#684
Conversation
hf
force-pushed
the
hf/fix-onauthstatechange-with-signed-in-onvisibilitychanged
branch
from
989a43b to
3497bce
Compare
| 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) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
| @@ -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) { | |||
There was a problem hiding this comment.
what does the isInitial param do here? can we document it above?
| @@ -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 | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Might have been https://github.com/supabase/gotrue-js/pull/684/files#diff-3522461172efd6058d6b8da62fc2d30d8b524d2b64894ea2c67218c52f7fdff5L1210 - definitely worth double checking
There was a problem hiding this comment.
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.
| @@ -660,10 +660,6 @@ export default class GoTrueClient { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Returns the session, refreshing it if necessary. | |||
There was a problem hiding this comment.
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
|
I need more time to figure this out. |
|
Found something else: in multi-tab this SIGNED_IN event is emitted to all other tabs so it may be a bit too much. |
|
Quick check: is this PR still relevant? |
Each time a tab became visible, the
SIGNED_INevent was emitted. Although this is normal, it's unnecessary overhead. In some implementations, it could result in unnecessary requests or UI re-renders.Fixes #579