-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Conversation
Handle empty as well as missing env variables
Re-added comments
Reduced line length and restored chaining operators just in case there's a good reason for including them.
@dalkommatt is attempting to deploy a commit to the Vercel Solutions Team on Vercel. A member of the Team first needs to authorize 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.
You've implemented a bunch of different bug fixes and features and opinionated refactorings in a single PR. Some great stuff here, but I would love to see this PR broken down a bit more for easier review.
It can definitely be broken down if needed. A lot of stuff is from existing PRs (including yours) just to close them out. I tried not to get too opinionated, rather just to deliver on originally intended features. The server actions for updating the name and email were already present, for example, just disabled. The main issue is that there wasn't a great way of providing success and error state messages. In the future I would advocate dropping the auth UI package and switching to server-side auth with toast success/error states to make the project more consistent. |
workflows for creating supabase client
constraint, change trial handling per stripe docs
Nextjs 14 supabase ssr
I spent most of the day reviewing your first couple commits and sorting them into three different branches, in some cases with slight changes. (Let me know how or if you want me to handle opening PRs from these branches, because I want you to get credit for your excellent work.)
The supabase-auth stuff looks good, though I haven't comprehensively tested it yet. Required some reading of docs, and I learned some things. I added a few more comments than you, but otherwise my branch is in line with your code. Your ui-refactor also looks good to me, although I've placed The stripe-checkout stuff I'm unsure about. When I try to change my API version in I also adjusted some of what you changed in the create-checkout route. I put the I haven't looked at your toast or password recovery code yet at all, but I am excited to dive into that and also do some live testing tomorrow, because those are features I've been wanting to add myself, and you are going to save me a lot of time! |
Thank you, yeah maybe let's see if Thor or Jon wants to take a look at the current PR first. My last PR was also quite big but did end up getting merged with some revisions. In the meantime are you able to suggest the changes here?
Not sure that I changed much as far as auth goes, it's still using supabase-provider.tsx with Auth UI. I think the main thing is that I switched sign out to a server action. Matching
Come to think of it I may have updated my Stripe version from the Stripe dashboard at some point. The reason I changed the version in stripe.ts is because I was getting a type error
Dropped cancel_url is my mistake, definitely should be corrected. Nice idea on the trial period too.
Yeah I think you'll find a useEffect in the toaster listening for URL param changes is quite an elegant solution for handling notifications globally with all the components going server side. Password recovery is not working like expected currently so be aware. I was hoping someone from Supabase could take a look at it. We're not supposed to @ people, right 😅 |
I'll see if I can help you debug the password recovery. Re: Stripe, I got
exactly the opposite type error.
…On Wed, Nov 29, 2023, 12:25 AM Matt ***@***.***> wrote:
I spent most of the day reviewing your first couple commits and sorting
them into three different branches, in some cases with slight changes. (Let
me know how or if you want me to handle opening PRs from these branches,
because I want you to get credit for your excellent work.)
Thank you, yeah maybe let's see if Thor or Jon wants to take a look at the
current PR first. My last PR was also quite big but did end up getting
merged with some revisions. In the meantime are you able to suggest the
changes here?
The supabase-auth stuff looks good, though I haven't comprehensively
tested it yet. Required some reading of docs, and I learned some things. I
added a few more comments than you, but otherwise my branch is in line with
your code. Your ui-refactor also looks good to me, although I've placed
Card.tsx into a Card subfolder and exported it from an index.ts file to
match the rest of the ui components.
Not sure that I changed much as far as auth goes, it's still using
supabase-provider.tsx with Auth UI. I think the main thing is that I
switched sign out to a server action. Matching Card with the other
components is a good change!
The stripe-checkout stuff I'm unsure about. When I try to change my API
version in stripe.ts to match yours, I get a type error. The type is
apparently set by my install of the latest version of the stripe library
in node_modules. It's possible that the installation process is somehow
pulling my account's API version from my Stripe dashboard. There probably
needs to be some documentation in the README about how to make sure the API
version specified in 'stripe.ts' matches the API version of one's Stripe
account and the API version permitted by the stripe module's type
definitions.
Come to think of it I may have updated my Stripe version from the Stripe
dashboard at some point. The reason I changed the version in stripe.ts is
because I was getting a type error Type '"2022-11-15"' is not assignable
to type '"2023-10-16"'.ts(2322) Do you not get that?
I also adjusted some of what you changed in the create-checkout route. I
put the cancel_url back in, so that Stripe checkout will provide a 'back'
button for the user to cancel checkout if they want. And instead of
dropping all support for free trials, I used 'trial_period_days:
price.trial_period_days,' which should dynamically allow for both trialing
and non-trialing subscription, as controlled from the Stripe dashboard.
(The Stripe docs say setting trial_end or trial_period_days is preferred
over setting trial_from_plan, though I can't see why, as the latter seems
more secure to me at a glance.)
Dropped cancel_url is my mistake, definitely should be corrected. Nice
idea on the trial period too.
I haven't looked at your toast or password recovery code yet at all, but I
am excited to dive into that and also do some live testing tomorrow,
because those are features I've been wanting to add myself, and you are
going to save me a lot of time!
Yeah I think you'll find a useEffect in the toaster listening for URL
param changes is quite an elegant solution for handling notifications
globally with all the components going server side. Password recovery is
not working like expected currently so be aware. I was hoping someone from
Supabase could take a look at it. We're not supposed to @ people, right 😅
—
Reply to this email directly, view it on GitHub
<#278 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ASCYPGN5TJYXYGYJ4LRICTTYG3BLRAVCNFSM6AAAAAA75KVAIOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMZRGI2DIMBQGY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
I'm fine with one large PR 👍 Thank you for opening this! |
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
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 👍 |
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.
This looks great! Awesome work everyone!
Super big shoutout and thank you to @dalkommatt and @chriscarrollsmith for doing a huge amount of work to get this one over the line! 🙌
Amazing work everyone 💯 |
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.