auth-js icon indicating copy to clipboard operation
auth-js copied to clipboard

Add initial event to onAuthStateChange

Open lsrugo opened this issue 2 years ago • 6 comments

What kind of change does this PR introduce?

Fixes supabase/gotrue-js#326

What is the current behavior?

There is no event fired unless a sign in occurs, but that is sometimes delayed. So there is no way to know if the user is signed in or not right when the page loads

What is the new behavior?

There is an INITIAL event fired after the library is finished loading and checking for a logged in user.

lsrugo avatar Oct 16 '21 00:10 lsrugo

looks like the failing tests are from gotrue - not your changes. Let me investigate

kiwicopple avatar Oct 18 '21 11:10 kiwicopple

The format of the session object changed on the backend. It returns an array for session.provider instead of a string. Should we update the tests to match that?

lsrugo avatar Oct 18 '21 19:10 lsrugo

Adding a bit more food for thought on the above: Calling .session() or .user() will return null if the session is expired, though it can be refreshed. So checking these when attaching the auth state listener won't work.

gustavohenke avatar Oct 19 '21 09:10 gustavohenke

Adding a bit more food for thought on the above: Calling .session() or .user() will return null if the session is expired, though it can be refreshed. So checking these when attaching the auth state listener won't work.

Maybe inside .session() or .user() we should also check if the loading is finished and wait for that before returning null

lsrugo avatar Oct 20 '21 02:10 lsrugo

Has this change been given more consideration? Would love to see it implemented

mryechkin avatar Nov 21 '21 02:11 mryechkin

https://github.com/supabase/gotrue-js/pull/147#issuecomment-946548042 @gustavohenke Could this be solved potentially but setting an isInitialized boolean on the client which can be checked at the same time as attaching listeners? Still not sure if this would be 100% atomic though (ie vs passing in a callback to the constructor).

Alternatively the client could present async isInitialized() as an async function which returns immediately if the state is already known, or awaits the finally {} in the constructor's Promise.all()? This would enable the second approach @lsrugo mentioned here: https://github.com/supabase/gotrue-js/pull/147#issuecomment-947259155


// ...
class GoTrueClient {
  private _isInitialized: boolean = false

  public constructor() {
    // ....
    Promise.all([
      this._recoverSession(),
      this._recoverAndRefresh(),
      this.getSessionFromUrl({ storeSession: true })
    ])
      .then(([, , { error }]) => {
        if (error) {
          console.error("Error getting session from URL.", error)
        }
      })
      .catch((error) => {
        console.error("Error recovering session.", error)
        // Any other error handling...
      })
      .finally(() => {
        this._notifyAllSubscribers("INITIALIZED") // Other events appear to be past-tense.
        this._isInitialized = true
      })
  }

  public async isInitialized(): Promise<boolean> {
    if (this._isInitialized) {
      return true
    }
    return new Promise((resolve) => {
      this.onAuthStateChange((event) => {
        if (event === "INITIALIZED") {
          return resolve(true)
        }
      })
    })
  }
}

prescience-data avatar Jan 08 '22 00:01 prescience-data

Thank you for this contribution, however the library has advanced quite a bit.

hf avatar Feb 02 '23 10:02 hf