supabase
fix: `_recoverAndRefresh` code efficiency and tidy up
What kind of change does this PR introduce?
Removes unneeded checks, other code and makes things slightly more efficient.
What is the current behavior?
Within _recoverAndRefresh(), we're checking for currentSession.expires_at and currentSession.refreshToken as part of a "hey is the session expired?" if block.
We've also left a _notifyAllSubscribers() call for SIGNED_IN, after a recent PR.
What is the new behavior?
currentSession.expires_at and currentSession.refresh_token are already checked for existence within _isValidSession(), so they've been removed from their respective if blocks. I can also understand if you'd still rather be explicit, even though that means they're getting checked twice.
We can also collapse a child if block and move it's remaining check for this.autoRefreshToken to the parent.
We also remove the SIGNED_IN emit event that happens towards the end of this function. In all other cases, _notifyAllSubscribers() is only called if a _saveSession() or _removeSession() call preceded it. The one exception is within constructor(), where an event listener, to emit events, is setup for multi-tab communication. The recent PR, referenced above, removed the _saveSession() call within _recoverAndRefresh(), so I'd argue there's no need to emit SIGNED_IN.
A possible anti-argument might be something like, "Emitting the SIGNED_IN event is just expressing that we noticed someone is signed in during recover and refresh". However, in the case of an expired session, we just refresh the token - causing a TOKEN_REFRESHED event - and then move on. There's no SIGNED_IN event emitted in this case. I'd say we either emit SIGNED_IN in both cases or not at all, but I lean towards not at all.
Additional context
Add any other context or screenshots.
I'm afraid we can't touch the events too much as they are an API contract. The event must remain, however inconvenient or inefficient.
As for the rest of the changes, we'd like to keep the changes in these functions for now within the team at Supabase as we're debugging some weird very rare behaviors with random logouts. I'll take a look at your changes and try to incorporate them in fixes as we go along. Sorry I don't want to discourage you, but this is one of those heisenbugs which we need consistency to figure out where it's coming from.
Understood. Happy bug-hunting!
I'm afraid we can't touch the events too much as they are an API contract. The event must remain, however inconvenient or inefficient.
I don't understand this part, however, but that's ok.