supabase / auth-js

An isomorphic Javascript library for Supabase Auth.
MIT License
365 stars 164 forks source link

Impossible to check null session without getSession warning #907

Open jdgamble555 opened 6 months ago

jdgamble555 commented 6 months ago

Bug report

Describe the bug

This is an auth.js issue related to #873, so I'm not going to link the other repos.

It is IMPOSSIBLE to check for a session without contacting the Supabase server or displaying the annoying warning.

To be more specific, if my session is NULL, there is NO reason for me to need to contact the Supabase server. This means, for my pages that don't require a login, it will ALWAYS contact the Supabase server needlessly.

The problem lies in the suppressGetSessionWarning can only be called when the _saveSession gets called. What if there is no session? Then we have to use getUser() for no reason, or deal with a needless warning OVER and OVER.

To Reproduce

Check a user's session with getSession() while not logged in, you get the warning.

Expected behavior

It is expected that we can use getSession() without a warning. The warning only shows up when there is a session and we can use getSession() without calling getUser().

Other Problems

This has been mentioned in all three github issues, but still not solved.

Please do not close this issue until both the NextJS and SvelteKit (preferably all docs) have been updated to NOT only use getUser() on the server for NULL.

Adding an extra needless fetch is a big deal.

J

j4w8n commented 6 months ago

The SvelteKit steps are updated - they call getSession() first, and if null then they return such and do not invoke getUser(). However, Next docs do not appear to show this.

Check a user's session with getSession() while not logged in, you get the warning.

Which framework are you having this issue with?

jdgamble555 commented 6 months ago

I'm using SvelteKit, but it doesn't matter which Framework you use, as you cannot call getSession first due to the _saveSession problem mentioned above.

If there is no session, you will get the warning always.

J

j4w8n commented 6 months ago

I'm using SvelteKit, but it doesn't matter which Framework you use, as you cannot call getSession first due to the _saveSession problem mentioned above.

If there is no session, you will get the warning always.

J

Ok, thanks. I assumed that's what you used, based on past interactions.

I ask because I'm using SvelteKit, but I don't have that specific issue when there's no one logged in. Do you have a repro?

jdgamble555 commented 6 months ago

Did you look at the core code and do you see the problem? Are you using getSession before getUser?

I can make a repo if necessary, but all you have to do is follow the SvelteKit guide.

The NextJS guide doesn't use getSession, which means you have to call the extra fetch with getUser.

j4w8n commented 6 months ago

Hmm... I have a test repo that follows the SvelteKit guide, here, but I never see logs unless there is a logged-in user.

jdgamble555 commented 6 months ago

I will check my repo when I get home from work.

However, if there is a logged-in user, why are you still seeing the logs unless it is fixed? If you're seeing it once, it will run multiple times due to how hooks.server.ts works in Svelte.

J

j4w8n commented 6 months ago

Yep, it logs multiple times for me. But only when there's a user logged in.

If I read your description correctly, you're also having issues when a user isn't logged in, correct?

P.S. When auth-js v2.64.3 drops, it will limit the logs; which isn't a great consolation, but makes it a bit less annoying.

jdgamble555 commented 6 months ago

Yep, it logs multiple times for me. But only when there's a user logged in.

I just checked my app on my home project, and it seems I jumped the gun. The warning is only shown when logged in. After re-looking at this code, I believe I see how this is possible on this line.

That means SvelteKit has the correct idea to use getSession() first, while NextJS is fetching needlessly. The NextJS Tutorial should be updated to call getSession() first, and only call getUser() when there is a session to verify. Again, extra fetches can be costly, even for Supabase.

P.S. When auth-js v2.64.3 drops, it will limit the logs; which isn't a great consolation, but makes it a bit less annoying.

  1. When will auth-js v2.64.3 drop?
  2. What do you mean limit the log? Shouldn't it ONLY be displayed when a user is calling it incorrectly (not verifying a session)?

J

j4w8n commented 6 months ago

I'm not sure about the version timeline. Likely this week.

The limit should keep things calmer. But we're never gonna get rid of it with the current state of SvelteKit and the Supabase ssr layout.ts cookie storage for SvelteKit.

The warning needs to die, but that likely won't happen until Supabase comes out with asymmetric jwts. I hope it happens at that point; or as silentworks suggested: at least don't log it in production.

fnimick commented 3 months ago

If you are calling getUser on requests and want to silence the warning as your session is guaranteed to be safe, see my approach in https://github.com/fnimick/sveltekit-supabase-auth-starter - I replace the user proxy object that triggers the warning with the user instance returned from getUser.