supabase / auth-js

An isomorphic Javascript library for Supabase Auth.
MIT License
318 stars 152 forks source link

fix: limit proxy session warning to once per client instance #900

Closed j4w8n closed 1 month ago

j4w8n commented 1 month ago

What kind of change does this PR introduce?

Bug fix.

What is the current behavior?

A call to getSession(), when using server storage, logs a warning every time session.user is accessed. This is causing a lot of people to see multiple consecutive logs to the console - especially for SvelteKit users.

What is the new behavior?

Ensures that accessing session.user, from a getSession() call, only logs the warning once per Proxy session instance. In other words, once per server request.

Additional context

https://github.com/supabase/auth-js/issues/888

Zanzofily commented 1 month ago

I believe you are just updating the local variable here, which won't be picked up on next invocations.

j4w8n commented 1 month ago

I believe you are just updating the local variable here, which won't be picked up on next invocations.

Perhaps I got it wrong, but it passes the addition to the test. That let creates a reference to this.suppress... on the client; so it should be changing the value of this.supress... when suppressWarning is changed. Then, when the value is checked again, only for the current server client, it should be true and not emit any further warnings - again, only for the current client.

This is only a per-request "fix".

Zanzofily commented 1 month ago

I believe you are just updating the local variable here, which won't be picked up on next invocations.

Perhaps I got it wrong, but it passes the addition to the test. That let creates a reference to this.suppress... on the client; so it should be changing the value of this.supress... when suppressWarning is changed. Then, when the value is checked again, only for the current server client, it should be true and not emit any further warnings - again, only for the current client.

This is only a per-request "fix".

References only work for objects in javascript, It's passing the test because you're using the value of session?.user rather than re-calling getSession() inside your test

j4w8n commented 1 month ago

I believe you are just updating the local variable here, which won't be picked up on next invocations.

Perhaps I got it wrong, but it passes the addition to the test. That let creates a reference to this.suppress... on the client; so it should be changing the value of this.supress... when suppressWarning is changed. Then, when the value is checked again, only for the current server client, it should be true and not emit any further warnings - again, only for the current client.

This is only a per-request "fix".

References only work for objects in javascript, It's passing the test because you're using the value of session?.user rather than re-calling getSession() inside your test

Ah thank you! I'll have to play with it a bit more then.

So, if I understand you, this is only a partial fix: it works only when you re-access session.user, but if you were to call getSession again, then it would log again.

kangmingtay commented 1 month ago

So, if I understand you, this is only a partial fix: it works only when you re-access session.user, but if you were to call getSession again, then it would log again.

yup that's because a new proxy is created everytime getSession is called and returns the non-expired session

j4w8n commented 1 month ago

Thanks @kangmingtay and @Zanzofily. I've made a couple of tweaks that I believe resolves the issue with subsequent proxy instance creations.

@kangmingtay, I'm not sure if the team approves of me expanding an existing test. I can separate if needed.

kangmingtay commented 1 month ago

@j4w8n that's fine, thanks for helping with this!