-
Notifications
You must be signed in to change notification settings - Fork 148
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: node middleware #725
Conversation
🦋 Changeset detectedLatest commit: a863666 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
great work on this Nico! one thing to note; Node.js middleware in |
9829e60
to
1ea55e7
Compare
options, | ||
); | ||
|
||
// Do we need to copy or do something with env file here? |
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.
Hmm I'm not sure. I can't think of anything obvious.
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.
Nice LGTM!
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.
LGTM.
One thing I did not realize is that Node and edge middlewares are exclusive.
Added a few minor comments.
@@ -50,7 +50,7 @@ export async function compileOpenNextConfig( | |||
// We need to check if the config uses the edge runtime at any point | |||
// If it does, we need to compile it with the edge runtime | |||
const usesEdgeRuntime = | |||
config.middleware?.external || | |||
(config.middleware?.external && config.middleware.runtime !== "node") || |
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.
comment: this might not play well with Cloudflare but we probably need to rethink how we compile to edge vs node
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 think we should rely on the wrapper
to tell us how we should compile (i.e. node vs workers).
For the moment i think it's fine, node middleware is still experimental
remove useless function
Co-authored-by: Victor Berchet <[email protected]>
413f661
to
a863666
Compare
This should be close to being ready
Couple of issues that needed to be resolved:
src
folder (fixed in Ensure src/middleware handles correctly vercel/next.js#75702)middleware.js
manually (fixed in https://github.com/vercel/next.js/pull/75765/files)