-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat!: Migrate to @supabase/ssr
#357
Conversation
👷 Deploy request for n3-supabase pending review.Visit the deploys page to approve it
|
There is another issue that comes up now:
This is actually something that should be changed in the library regardless of whether this PR is merged. EDIT: After upgrading |
Previously, the following error was thrown Failed to execute 'postMessage' on 'BroadcastChannel': #<Object> could not be cloned.
Hello @felixgabler, I' don't have a lot of time for this module since I've created it for a side project that do not need evolution no more and I'm so happy when people help me to maintain it and make it evolve, thanks for that! Concerning the PR, I faced two blocking points:
After reading the Supabase docs concerning the Those changes include breaking changes but it make sense to release a major version once this PR is merged to be sure users make some complete test to upgrade the module. Tell me what you think of this and if it's working well on your project. Also I want to get rid of the |
That all sounds very reasonable and I welcome the changes you made to keep things more secure! Very cool.
We deployed my branch and have not faced issues so far. It actually fixed some bugs (we set cookies on the top-level domain which the subdomains inherit and I suspect something broke around this but works now with
That is weird.. I don't get them anymore after the upgrade. Perhaps you need to delete the |
If
A simple example would be fetching the user profile with |
I pushed two new fix:
|
I tried to play with |
I created a branch with a repro of the warning: https://github.com/felixgabler/supabase-nuxt/tree/warning-repro The warning is shown when navigating between However, I might have figured out what the problem is. Does "useAsyncData is a composable meant to be called directly in the Nuxt context" (Nuxt Docs) mean that these composables can't be reused in other composables? That would be somewhat annoying but would at least explain the warning.. If this is indeed true, do you know what the "right" way would be to share this user state? |
Indeed I don't think that calling |
Let's closely follow this issue. Once it's fixed, we can merge this PR . Wdyt? |
We put useAsyncData in a composable because we have many pages/components in our app that work on the same underlying data. The problem is that there is not really one component that is always there which could become the owner of the useAsyncData, where we could update a shared state. This is where the reusability of composables really comes in handy. Does that make sense? |
Sounds good to me! |
Maybe you can try with this experimental feature: https://nuxt.com/docs/guide/going-further/experimental-features#asynccontext Or what about calling the |
Unfortunately, this does not remove the warnings. The good thing right now is that at least it works in SPA mode. It is only that the warnings are annoying but they can be lived with.
The issue here is that we also don't want to load all data if it won't be used. So calling it inside a root page or plugin is also not optimal. The only way I can think of making the Especially given that the convention is that all composables (i.e., anything with |
I wonder if it fell off the plate. Perhaps best to create a new issue? |
@atinux could I perhaps also get your thoughts on this? I was quite surprised to learn that |
Indeed I've create a new one: supabase/auth-js#912 |
} | ||
// We rely on `getSession` on the client. | ||
else { | ||
const session = await useSupabaseSession() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we already get the user info for the server in the plugin, does it make sense to also do the same for the client plugin? This would clean up this composable a bit and we also have the onAuthStateChange
listener there already, so it would be a fitting place.
Cannot really think of reasons why not to do it. I am not sure if there is value in refreshing the userState each time useSupabaseUser is called. It might even be bad for performance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my point of view, onAuthStateChange
is only triggered when Supabase API is called to refresh the session either when getSession
or refreshSession
are called or when user is logged out. But on client side, when navigating, we want to ensure that the session (and the user) are still connected (token not expired) that's why calling getSession
ensures this.
Important to note that getSession
is not calling Supabase API when token are not expired (most of the time) but only reading local storage, this is not a lack of performance thus.
However, if you want to make some tests and prove that onAuthStateChange
is triggered when token is expired and not refresh, we could remove it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I can tell, the onAuthStateChange
is called on events that would cover token expiration because SIGNED_OUT
is also called when the token expires. Reading the docs, it definitely looks to me like one can rely on these auth events to track sessions. We also might not even need to call a getSession
to initialize since the INITIAL_SESSION
auth event triggers after client creation.
I can do more testing later today if you'd like
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case we can indeed remove the async call to useSupabaseSession
and get useSupabaseUser
back to a synchronous composable. Which is a good thing meaning no breaking changes!
You can update the code then we'll both update our personal projects to ensure it works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
useSupabaseUser
will be back to the old version only returning the state. State being muted by either the server plugin or the onAuthStateChange
. It makes sense to me. useSupabaseSession
stays as a standalone composable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixgabler If you confirm everything works smoothly with your project, let's push your changes in this PR. It looks all good to me! I'll test it on mine once you push your changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've been running it for the last few days without issues related to the changes. We do get the supabase useSession
warnings which are annoying but not a problem. Looking forward to hearing how it works on your project :)
EDIT: One issue we did encounter is a header overflow:
ERROR [h3] [unhandled] Parse Error: Header overflow 9:40:44 AM
at Socket.socketOnData (node:_http_client:540:22)
at Socket.emit (node:events:519:28)
at addChunk (node:internal/streams/readable:559:12)
at readableAddChunkPushByteMode (node:internal/streams/readable:510:3)
at Readable.push (node:internal/streams/readable:390:5)
at Pipe.onStreamRead (node:internal/stream_base_commons:190:23)
This is most likely due to large auth and refresh tokens in the Supabase headers.
I've since increased the max header size (NODE_OPTIONS=--max-http-header-size=32384
) and hopefully it is resolved now. Did you encounter this too? If so, we might have to add something to the docs about increasing the header size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed the new version on my side project. I'll tell you if everything's ok :)
This warning is really annoying, they do not seem to be in a hurry with it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! It would be awesome if we could move this forward. I don't think they will remove the warning as it is generally unsafe to read the user from the session directly.
The warning only shows on server side if session.user
is accessed (because this is unsafe behavior). While this PR does not actually read the user from the session, we do write the session using useState
(see this line). Any serialization of the session would also cause it to evaluate its user
field, therefore triggering the warning.
I propose to make a breaking change and making useSession only work client side. I don't really see any other answer because Supabase (rightly, I believe) will not remove this warning and at the same time, the latest changes to @supabase/ssr
are super important and full of bug fixes. My project runs much smoother since we switched.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I missed this one @felixgabler sorry. Then we're aligned (see my last message on the main thread). Let's merge it. My opinion is not to create breaking changes and just release it with the warning. People will complain about it and it might add upvotes to our issue so that supabase maintainers will address it!
…rough plugin instead
Control session through plugin
Opening another thread, I have also added a branch to my fork that adds |
Hi @felixgabler, I read your comment and since I've already encountered this problem, I thought I'd reply. I came across this video by Alexandre Lichter core Nuxt team member, which I recommend you watch. It explains that I don't know exactly what you want to do in your case, but to share the data and be able to use it just where it's needed I use Pinia stores, with actions that contain API calls using I find that stores are more efficient than composables for managing data coming from API requests, because they allow access to their state anywhere in the Nuxt app easily. Anyway, sorry for the off topic, I wanted to thank you both for the work you've done to move to @supabase/srr. I've been having a big problem for some time in my Nuxt app, my users' Supabase session is lost after a while, and in my When I did some research, I saw that it happened to other people, but I didn't see any solutions. Thanks again for your efforts |
@felixgabler, I will merge this PR. Users have been raising concerns about outdated dependencies and other issues within the module. My intention was to wait for this merge, as managing updates across two separate branches is hard but I can't wait no more. The warning mentioned is non-disruptive and does not affect functionality, so merging this now should help resolve some of the existing issues, including those you've reported @XStarlink. Additionally, if users continue to encounter the warning, they can upvote the issue and it might encourage the supabase maintainers to address it. Concerning your Once again, thanks for your help on this PR 💚 |
Started using
@supabase/ssr
to create clients, simplifying cookie handling.https://supabase.com/docs/guides/auth/server-side/creating-a-client
Types of changes
Description
Resolves: #301
I replaced the plain usage of
createClient
(inserverSupabaseClient
,supabase.server.ts
, andsupabase.client.ts
) with the more advancedcreateServerClient
andcreateBrowserClient
from@supabase/ssr
. In the process, I removed some of the manual cookie handling since it should be taken care of by Supabase'sauth.storage
setting. This is based on my limited understanding though, so would appreciate a confirmation.This was because we were encountering quite a few issues of cookies getting lost somewhere. Now everything seems to work fine again.
I was not entirely sure what to do about the
clientOptions
since they should be managed by@supabase/ssr
now. Also, thecookieName
is now also quite irrelevant and I did not find a good way to include it in the new setup for the other cookies. One idea would be to prefix them all again through thecookies
option functions?Checklist: