Next.js 14 & Supabase SSR

dalkommatt commented 10 months ago

Closes #115 #190 #200 #222 #244 #254 #262 #271 #248 #223 #274

This PR updates all packages and switches from the now deprecated auth helpers to Supabase SSR.

Name and email updating is working, but if Jon or Thor or someone wants to look at password reset (account/recover-password) that would be awesome.
Added shadcn UI toast for error and status handling
Heads up, I have also encountered this issue https://github.com/supabase/auth-helpers/issues/696 with Supabase SSR.

chriscarrollsmith commented 8 months ago

@dijonmusters actually the problem of redundant API calls was much worse than I described above. The Supabase 'auth' API log showed only two calls per page load, but the 'edge' API log showed a lot more than that. When I added a console log in the middleware to see what routes it was being rendered on, the reason for the problem became obvious:

Calling middleware for route: / Calling middleware for route: /vercel.svg Calling middleware for route: /nextjs.svg Calling middleware for route: /stripe.svg Calling middleware for route: /supabase.svg Calling middleware for route: /github.svg

Accordingly, I updated the pattern matcher:

export const config = {
  matcher: [
    /*
     * Match all request paths except for the ones starting with:
     * - _next/static (static files)
     * - _next/image (image optimization files)
     * - favicon.ico (favicon file)
     * Feel free to modify this pattern to include more paths.
     */
    '/((?!_next/static|_next/image|favicon.ico|.*\\.(?:svg|png|jpg|jpeg|gif|webp)$).)*',
  ],
}

So that issue, at least, has been resolved. Way fewer edge API calls being logged now. And it looks like the '401: invalid claim: missing sub claim' log entry is just the expected response of Supabase's 'user' API endpoint when the user is not logged in, so that's not a problem.

I am still getting two API requests to Supabase's auth API when I load the Pricing page. One is from the middleware, and one is from the user API when we conditionally render the page based on whether the user is logged in. So there's still some redundancy there, but it's probably unavoidable, and I think the change to the pattern matcher will mostly resolve the rate-limit problem I was running into.

As far as I'm concerned, this PR is ready to be merged. The only question still to be resolved is how you want to reconcile @dalkommatt's version with mine.

(By the way, sorry for taking so long to resolve this. Life's been busy lately!)

dijonmusters commented 8 months ago

@dijonmusters actually the problem of redundant API calls was much worse than I described above. The Supabase 'auth' API log showed only two calls per page load, but the 'edge' API log showed a lot more than that. When I added a console log in the middleware to see what routes it was being rendered on, the reason for the problem became obvious:

Calling middleware for route: / Calling middleware for route: /vercel.svg Calling middleware for route: /nextjs.svg Calling middleware for route: /stripe.svg Calling middleware for route: /supabase.svg Calling middleware for route: /github.svg

Accordingly, I updated the pattern matcher:
export const config = {
  matcher: [
    /*
     * Match all request paths except for the ones starting with:
     * - _next/static (static files)
     * - _next/image (image optimization files)
     * - favicon.ico (favicon file)
     * Feel free to modify this pattern to include more paths.
     */
    '/((?!_next/static|_next/image|favicon.ico|.*\\.(?:svg|png|jpg|jpeg|gif|webp)$).)*',
  ],
}
So that issue, at least, has been resolved. Way fewer edge API calls being logged now. And it looks like the '401: invalid claim: missing sub claim' log entry is just the expected response of Supabase's 'user' API endpoint when the user is not logged in, so that's not a problem.

I am still getting two API requests to Supabase's auth API when I load the Pricing page. One is from the middleware, and one is from the user API when we conditionally render the page based on whether the user is logged in. So there's still some redundancy there, but it's probably unavoidable, and I think the change to the pattern matcher will mostly resolve the rate-limit problem I was running into.

As far as I'm concerned, this PR is ready to be merged. The only question still to be resolved is how you want to reconcile @dalkommatt's version with mine.

(By the way, sorry for taking so long to resolve this. Life's been busy lately!)

Awesome work! I will need to update that pattern in other templates and docs pages too! 🙌

This looks great! I think it's ready to merge!

@dalkommatt are you okay if we merge this one, and then open a separate PR to add anything that's missing? Is it just the magic link option, or have these significantly diverged?

dalkommatt commented 8 months ago

@dijonmusters actually the problem of redundant API calls was much worse than I described above. The Supabase 'auth' API log showed only two calls per page load, but the 'edge' API log showed a lot more than that. When I added a console log in the middleware to see what routes it was being rendered on, the reason for the problem became obvious:

Calling middleware for route: / Calling middleware for route: /vercel.svg Calling middleware for route: /nextjs.svg Calling middleware for route: /stripe.svg Calling middleware for route: /supabase.svg Calling middleware for route: /github.svg

Accordingly, I updated the pattern matcher:
export const config = {
  matcher: [
    /*
     * Match all request paths except for the ones starting with:
     * - _next/static (static files)
     * - _next/image (image optimization files)
     * - favicon.ico (favicon file)
     * Feel free to modify this pattern to include more paths.
     */
    '/((?!_next/static|_next/image|favicon.ico|.*\\.(?:svg|png|jpg|jpeg|gif|webp)$).)*',
  ],
}
So that issue, at least, has been resolved. Way fewer edge API calls being logged now. And it looks like the '401: invalid claim: missing sub claim' log entry is just the expected response of Supabase's 'user' API endpoint when the user is not logged in, so that's not a problem. I am still getting two API requests to Supabase's auth API when I load the Pricing page. One is from the middleware, and one is from the user API when we conditionally render the page based on whether the user is logged in. So there's still some redundancy there, but it's probably unavoidable, and I think the change to the pattern matcher will mostly resolve the rate-limit problem I was running into. As far as I'm concerned, this PR is ready to be merged. The only question still to be resolved is how you want to reconcile @dalkommatt's version with mine. (By the way, sorry for taking so long to resolve this. Life's been busy lately!)
Awesome work! I will need to update that pattern in other templates and docs pages too! 🙌

This looks great! I think it's ready to merge!

@dalkommatt are you okay if we merge this one, and then open a separate PR to add anything that's missing? Is it just the magic link option, or have these significantly diverged?

@dijonmusters I say go ahead and merge and then work from a new PR yeah 👍Chris's PR also has password login/reset, right @chriscarrollsmith ?

chriscarrollsmith commented 8 months ago

Password login/reset (which can be turned on or off with a flag) is the main difference, but I've tinkered with lots of other stuff too. Added support for local development with Supabase, moved the API endpoints to server actions, fixed lots of minor bugs, that kind of thing.

I've back-merged all but your latest commit, so the only reconciliation needed will be in how we handle comparing the new and old email when it's changed from the account page. You handled this with an error toast, while I opted to reject the request without one. Also, I believe you handled it server-side, while I handled it on the client.

Other than that commit, I don't think you'll have to resolve a lot of conflicts. (Git might have other ideas, idk.) If you don't want to deal with the git merge, @dijonmusters can just accept the other open PR and close this one. Like I said, I pulled all your changes into it, so you will still get credit for your work.

dijonmusters commented 8 months ago

Oh right! I didn't realise we were talking about two divergent branches. Sounds like there is quite a bit in Chris' PR. @dalkommatt how do you feel about resolving those conflicts and merging that into this branch - would be good to keep this history of conversation with the PR that gets merged. Then @thorwebdev and myself can do one final review and get this mega chunk of awesomeness merged in once and for all! 🎉

dijonmusters commented 8 months ago

@chriscarrollsmith can you also update the matcher pattern from

'/((?!_next/static|_next/image|favicon.ico|.*\\.(?:svg|png|jpg|jpeg|gif|webp)$).)*'

to

'/((?!_next/static|_next/image|favicon.ico|.*\\.(?:svg|png|jpg|jpeg|gif|webp)$).*)'

Just noticed that last star is in the wrong place so it doesn't match any route ever 😬

dijonmusters commented 8 months ago

@chriscarrollsmith can you also update the matcher pattern from
'/((?!_next/static|_next/image|favicon.ico|.*\\.(?:svg|png|jpg|jpeg|gif|webp)$).)*'
to
'/((?!_next/static|_next/image|favicon.ico|.*\\.(?:svg|png|jpg|jpeg|gif|webp)$).*)'
Just noticed that last star is in the wrong place so it doesn't match any route ever 😬

Added a PR to fix this one: https://github.com/dalkommatt/nextjs-subscription-payments/pull/3

Thanks for merging those changes in @dalkommatt 🙌

dijonmusters commented 8 months ago

Another PR to simplify creating a Supabase client server-side, and running prettier over the entire project - sorry for the number of lines changed, but most of them should overlap with the changes in this PR 👍

tmanager2025 commented 8 months ago

@dijonmusters @dalkommatt where is supabase.auth.onAuthStateChange listener? When user signs out from one browser window it's basically not redirecting from another windows. Should not we have supabase.auth.onAuthStateChange listener in middleware to log out user from other browser tabs/window.

onAuthStateChange is no longer required for the auth flow as we are doing everything server-side. We are explicitly redirecting after the user signs in in /app/auth/callback/route.ts and after the user signs out in /components/ui/Navbar/Navbar.tsx. Previously the auth listener was required as it was the only way to know when the authentication process had completed. That is not the case with the code exchange route in the PKCE flow used by the auth-helpers and ssr packages 👍

Have same question as @tmanager2025. How to log out users from all other logged in tabs, when they signed out from one tab. Should not we listen to onAuthStateChange event to do this.

The onAuthStateChange function should work to solve that specific use case, but not sure this problem needs to be solved for this template - every line of code included increases the complexity and maintenance burden for everyone that uses it. It is pretty common for auth state to be checked on navigation or refresh, so if someone has logged out in a different tab, it should be expected that the app running in another tab is in a different state until they refresh or navigate somewhere that requires authentication. I would recommend leaving this out of the template, and up to the individual building the app if this is how they want to handle their logout flow 👍

@dijonmusters I agree with @myorgmanager, this is reference for many people for Login and Subscription. Having no way to Logout users on another tabs/window is bad once they logout.

Would love to see have this feature in this PR which is removed and we lost that functionality.

It is important to show how this done with RSC and supabase.

@dijonmusters @dalkommatt @chriscarrollsmith Any way you could add the functionality of logging out in other windows/tabs while user logs out from another window in RSC.

Would love to see this change also get added as many requested.

Thanks

chriscarrollsmith commented 8 months ago

Just noticed that last star is in the wrong place so it doesn't match any route ever 😬

Added a PR to fix this one: dalkommatt#3

Thanks for merging those changes in @dalkommatt 🙌

Thanks for catching this. Clearly I didn't test enough.

chriscarrollsmith commented 8 months ago

@dijonmusters @dalkommatt @chriscarrollsmith Any way you could add the functionality of logging out in other windows/tabs while user logs out from another window in RSC.

Would love to see this change also get added as many requested.

Thanks

Honestly, this has been open so long, I'd like to just see it get merged. We can work on your issue in a separate PR.

dijonmusters commented 8 months ago

@dijonmusters @dalkommatt @chriscarrollsmith Any way you could add the functionality of logging out in other windows/tabs while user logs out from another window in RSC. Would love to see this change also get added as many requested. Thanks

Honestly, this has been open so long, I'd like to just see it get merged. We can work on your issue in a separate PR.

Yep, I agree! Let's move any additional functionality to a separate follow-up PR 👍

@thorwebdev and I will review and test today, if any one else could try the whole flow end-to-end (setting up Stripe etc) that would be super helpful, as these changes are pretty extensive 😬

thorwebdev commented 7 months ago

I just tested this with a new Supabase and a new Stripe account, and it works like a charm — great work y'all 💚

chriscarrollsmith commented 7 months ago

Fuller list of issues and pull requests this PR should close:

ehowey commented 7 months ago

Just a note to say that I have been using this PR/branch as a base for a new project without issue. Nice work!!

laurosilvacom commented 7 months ago

Testing this out in a new project for my client! It runs SO smoothly 🚀 awesome work y'all!

dijonmusters commented 7 months ago

Awesome work everyone! I think this is ready to merge! 🎉

aroonjawaid commented 7 months ago

Just some feedback, while testing this pr. I noticed that after subscribing to a plan and then directly navigating to the homepage, clicking on "manage" in the pricing section allows me to subscribe again to the same plan.

chriscarrollsmith commented 7 months ago

Just some feedback, while testing this pr. I noticed that after subscribing to a plan and then directly navigating to the homepage, clicking on "manage" in the pricing section allows me to subscribe again to the same plan.

Thanks so much for testing! I will see what I can do to fix that bug.

dijonmusters commented 7 months ago

Just some feedback, while testing this pr. I noticed that after subscribing to a plan and then directly navigating to the homepage, clicking on "manage" in the pricing section allows me to subscribe again to the same plan.

Thanks so much for testing! I will see what I can do to fix that bug.

Looks like you can resolve this at the Stripe level now: https://stripe.com/docs/payments/checkout/limit-subscriptions

We could just add a step to the Setting up the customer portal section of the README.

This could either be done in this PR or a future one 👍

leerob commented 7 months ago

Amazing work everyone 💯

vercel / nextjs-subscription-payments

Next.js 14 & Supabase SSR #278