RFC: change localstorage in constructor to synchronous request

[kiwicopple]

Feature request

Is your feature request related to a problem? Please describe.

https://github.com/supabase/gotrue-js/blob/cccc4f49f6de8067b68c6892fa471a5dd544d4cb/src/GoTrueClient.ts#L57

Since the call to _recoverSession is async, it means that currentSession and currentUser isn't populated by the constructor, and that clients need to wait for the stateChangeEmmitter to fire.

This is causing a lot of problems [1] [2]

Describe the solution you'd like

LocalStorage is synchronous, so we can remove the await. However we will need a way to handle it for ReactNative.

Is there a way we can handle it for both situations?

Additional context

Side note - I have seen some people complaining that globalThis is undefined. This is also related to storage. Should this also be updated to something that works everywhere?

https://github.com/supabase/gotrue-js/blob/cccc4f49f6de8067b68c6892fa471a5dd544d4cb/src/GoTrueClient.ts#L10

[kiwicopple]

@ykdojo @thorwebdev Any thoughts on this one? I guess we might be able to detect if the storage is async somehow? Or we can provide some sort of sync storage for ReactNative users?

[ykdojo]

I wonder if we can make two versions of _recoverSession() (async and sync), and decide which one to use by detecting whether or not the user's using React Native here:

https://github.com/supabase/gotrue-js/blob/cccc4f49f6de8067b68c6892fa471a5dd544d4cb/src/GoTrueClient.ts#L57

[kiwicopple]

I like that idea @ykdojo

Not sure how we can detect if local storage is async, but i guess can run the sync function from the constructor and then let it fail silently.

[yigitest]

SupabaseClient uses internal auth.session and auth user to set authorization headers. But they are always set to null for initial calls.

While waiting for this issue to be fixed, is there another way to set authorization/bearer headers?

[thorwebdev]

To be clear, on web this._recoverSession() is currently synchronous because localStorage reads on web are synchronous, so here the await shouldn't have any effect. Only if we're running on a non-web platform like React Native and using async storage this should have an effect.

So not entirely sure if there's a different issue at play here.

[thorwebdev]

Also, you will need the onAuthStateChange event handler anyway, because we might be recovering the session from the refresh token in which case and async server request is involved: https://github.com/supabase/gotrue-js/blob/master/src/GoTrueClient.ts#L366, so there really isn't a way around having that handler. So I wonder if this is more about better documentation on the onAuthStateChange handler?

[yigitest]

this.__recoverSession() is currently is async because it is defined as aysnc and has await in it. When you call this._recoverSession() from the GoTrueClient::constructor(), it will be called, executed and finished some time in the future. Maybe 0ms from now, maybe 100ms.

I'm not an experienced JS developer. Maybe you could be right in this case. It depends on the implementation. But this is how concurrency in computer systems in general works.

Edit: Learned something new today <3

async function is_async(val){
    console.log("is_async", val);
    a();
    if(val){
        await b(); 
        // redundant await on sync function
        // but still makes rest of the method async
        // >> start a b end c
    }else{
        b(); 
        // >> start a b c end
    }
    c();
}

function a(){
    console.log("a");
}

function b(){
    console.log("b");
}

function c(){
    console.log("c");
}

console.log("start");
is_async(true);
console.log("end");

[drgarlic]

Hi, any news ?

It's really an annoying bug, can't just check if something is written in LocalStorage since it might be outdated

To be clear I'm using a Vue 3 SPA client side rendered and the first call still returns null

[kiwicopple]

I coded up a solution here, with this caveat:

https://github.com/supabase/gotrue-js/pull/68/files#r598291376

I don't know if I would call it a bug - in many cases we need to verify that the token is still valid, and if it's not valid we need to make a request to the server to refresh the token. This requires an API call, so there is no way we can synchronously return a new session from the constructor.

This change will solve any problems that people are having with refreshing the page however. If there is a valid session (set up within the last 60 mins) it will be immediately returned from the constructor