-
Notifications
You must be signed in to change notification settings - Fork 66
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
[auth-nextjs] Async Request APIs, compat with Next.js 15 #1137
Conversation
Thank you, we'll take a look soon! |
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.
Thanks so much for taking on this mechanical work! It's been on our "next up" list for a few weeks now.
@@ -153,8 +159,9 @@ export abstract class NextAuth extends NextAuthHelpers { | |||
return { | |||
GET: async ( | |||
req: NextRequest, | |||
{ params }: { params: { auth: string[] } }, | |||
ctx: { params: Promise<{ auth: string[] }> }, |
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.
Will this work in <15 or will those users need to do a type-cast?
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.
Good question. In runtime it shouldn't be a problem because it would just be awaiting a non-Promise; and i didn't notice any type error when i tried it in <15, but i can check again jic.
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.
Yeah, I'm not worried too much about runtime, and even if it breaks backwards compatibility, I'm fine with shipping it, I just want to understand everything that might break here so we can update the docs.
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.
Well the GET
is exported in a route.ts
to act as a Next.js Route Handler but it's not type checked against anything. I can run and build the project running Next.js before their breaking change with this version, no errors. And i don't know of any way that someone could be type-checking this and get a warning/error after this change.
At most we could add a small note to the docs referencing the change introduced by Next.js 15 and how this small library targets the new behaviour but supports both.
No worries! Glad to help and love what you guys are doing. I'm working on a sizable project by myself, so these kinds of helper libraries that help me ship faster are very welcome. And i'm sure you're quite busy with |
👋🏼 Small update: React 19 stable finally dropped, and with it a stable release of |
This PR adapts the
@edgedb/auth-nextjs
package for the [kinda*] breaking change of Next.js 15, where Request APIs likecookies
andparams
are now async.To solve this, i simply added
await
to all corresponding places. Awaiting something sync is not strictly a noop, but for these cases it shouldn't change behavior afaict.Tested in my own project with both Next.js before their breaking change and after it.
Breaking change: As
NextAppAuth.getSession()
has to awaitcookies()
, it is now anasync
function, which means those upgrading have to addawait
when calling it.This necessitates that
getSession
is now async, which is a breaking change, because it accesses the cookies.*kinda because as a compatibility layer it is still possible to not await it, but it's not what you want to do.