-
-
Notifications
You must be signed in to change notification settings - Fork 421
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
fix: disable edge runtime by default to enable windows development #1281
base: main
Are you sure you want to change the base?
Conversation
nickreynolds
commented
Jan 6, 2025
- Because of the bug described here, I updated the line that exports runtime to check if the process is being run on Windows and select the nodejs runtime in that case.
- I also updated the README to explain this, since it could otherwise be confusing, and my changes should be removed if that bug is fixed.
- fixes bug: Internal Server Error when running nextjs app locally on Windows #1280
@@ -3,7 +3,7 @@ import { NextRequest, NextResponse } from "next/server"; | |||
|
|||
import { handlers, isSecureContext } from "@acme/auth"; | |||
|
|||
export const runtime = "edge"; | |||
export const runtime = process.platform === "win32" ? "nodejs" : "edge"; |
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.
Pretty sure This doesn't work as this string need to be statically analyzavlr?
We can just remove it though if it's causing issues?
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.
it appears to work for me locally (i.e. I'm able to develop on Windows with this change) and my vercel deployments seem fine as well (although I'm not sure if they're using the correct runtime now).
As far as I can tell there are 3 options:
- update runtime export as done in this PR
- remove runtime export from all pages
- leave all code as-is and just add note for Windows developers
I defer to you on the best approach moving forward though, and happy to update this PR at your discretion.
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.
it appears to work for me locally (i.e. I'm able to develop on Windows with this change) and my vercel deployments seem fine as well (although I'm not sure if they're using the correct runtime now).
if this is the case, it means you're using different runtime in prod vs dev which sounds scary. I prefer no. 2
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 point. If you think that having windows-compatibility-by-default is more important than edge-runtime-by-default, I can do no. 2. Otherwise, I'd do no. 3. Do you want me to proceed with option 2?
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 tried using this before and it wouldn't deploy on vercel. This is only an issue on webpack through, if you are using turbopack this dissapears.
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.
yes, I was incorrect about my original solution working, my IDE just wasn't reporting the error, and the line was being ignored entirely (which meant it was functionally equivalent to deleting the line, at least for local dev, but obviously not the right solution)
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.
Do you want me to proceed with option 2?
Yes
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.
@juliusmarminge I made the changes to this PR that we discussed. Do you think we need to update the drizzle.config.ts
as well? I'm not particularly familiar with this, but should we switch to a pooled DB URL now that we aren't using edge runtime?
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 don't want a pooled url when running migrations/schema changes
49ed338
to
9bb20b5
Compare
9bb20b5
to
4154274
Compare