-
Notifications
You must be signed in to change notification settings - Fork 27.8k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix(edge): error handling for edge route and middleware is inconsiste…
…nt (#38401) ## What’s in there? This PR brings more consistency in how errors and warnings are reported when running code in the Edge Runtime: - Dynamic code evaluation (`eval()`, `new Function()`, `WebAssembly.instantiate()`, `WebAssembly.compile()`…) - Usage of Node.js global APIs (`BroadcastChannel`, `Buffer`, `TextDecoderStream`, `setImmediate()`...) - Usage of Node.js modules (`fs`, `path`, `child_process`…) The new error messages should mention *Edge Runtime* instead of *Middleware*, so they are valid in both cases. It also fixes a bug where the process polyfill would issue a warning for `process.cwd` (which is `undefined` but legit). Now, one has to invoke the function `process.cwd()` to trigger the error. It finally fixes the react-dev-overlay, where links from middleware and Edge API route files could not be opened because of the `(middleware)/` prefix in their name. About the later, please note that we can’t easily remove the prefix or change it for Edge API routes. It comes from the Webpack layer, which is the same for both. We may consider renaming it to *edge* instead in the future. ## How to test? These changes are almost fully covered with tests: ```bash pnpm testheadless --testPathPattern runtime-dynamic pnpm testheadless --testPathPattern runtime-with-node pnpm testheadless --testPathPattern runtime-module pnpm testheadless --testPathPattern middleware-dev-errors ``` To try them out manually, you can write a middleware and Edge route files like these: ```jsx // middleware.js import { NextResponse } from 'next/server' import { basename } from 'path' export default async function middleware() { eval('2+2') setImmediate(() => {}) basename() return NextResponse.next() } export const config = { matcher: '/' } ``` ```jsx // pages/api/route.js import { basename } from 'path' export default async function handle() { eval('2+2') setImmediate(() => {}) basename() return Response.json({ ok: true }) } export const config = { runtime: 'experimental-edge' } ``` The expected behaviours are: - [x] dev, middleware/edge route is using a node.js module: error at runtime (logs + read-dev-overlay): ```bash error - (middleware)/pages/api/route.js (1:0) @ Object.handle [as handler] error - The edge runtime does not support Node.js 'path' module. Learn More: https://nextjs.org/docs/messages/node-module-in-edge-runtime > 1 | import { basename } from "path"; 2 | export default async function handle() { ``` - [x] build, middleware/edge route is using a node.js module: warning but succeeds ```bash warn - Compiled with warnings ./middleware.js A Node.js module is loaded ('path' at line 4) which is not supported in the Edge Runtime. Learn More: https://nextjs.org/docs/messages/node-module-in-edge-runtime ./pages/api/route.js A Node.js module is loaded ('path' at line 1) which is not supported in the Edge Runtime. Learn More: https://nextjs.org/docs/messages/node-module-in-edge-runtime ``` - [x] production, middleware/edge route is using a node.js module: error at runtime (logs + 500 error) ```bash Error: The edge runtime does not support Node.js 'path' module. Learn More: https://nextjs.org/docs/messages/node-module-in-edge-runtime at <unknown> (file:///Users/damien/dev/next.js/packages/next/server/web/sandbox/context.ts:149) ``` - [x] dev, middleware/edge route is using a node.js global API: error at runtime (logs + read-dev-overlay): ```bash error - (middleware)/pages/api/route.js (4:2) @ Object.handle [as handler] error - A Node.js API is used (setImmediate) which is not supported in the Edge Runtime. Learn more: https://nextjs.org/docs/api-reference/edge-runtime 2 | 3 | export default async function handle() { > 4 | setImmediate(() => {}) | ^ ``` - [x] build, middleware/edge route is using a node.js global API: warning but succeeds ```bash warn - Compiled with warnings ./middleware.js A Node.js API is used (setImmediate at line: 6) which is not supported in the Edge Runtime. Learn more: https://nextjs.org/docs/api-reference/edge-runtime ./pages/api/route.js A Node.js API is used (setImmediate at line: 3) which is not supported in the Edge Runtime. Learn more: https://nextjs.org/docs/api-reference/edge-runtime ``` - [x] production, middleware/edge route is using a node.js module: error at runtime (logs + 500 error) ```bash Error: A Node.js API is used (setImmediate) which is not supported in the Edge Runtime. Learn more: https://nextjs.org/docs/api-reference/edge-runtime at <unknown> (file:///Users/damien/dev/next.js/packages/next/server/web/sandbox/context.ts:330) ``` - [x] dev, middleware/edge route is loading dynamic code: warning at runtime (logs + read-dev-overlay) and request succeeds (we allow dynamic code in dev only): ```bash warn - (middleware)/middleware.js (7:2) @ Object.middleware [as handler] warn - Dynamic Code Evaluation (e. g. 'eval', 'new Function') not allowed in Edge Runtime 5 | 6 | export default async function middleware() { > 7 | eval('2+2') ``` - [x] build, middleware/edge route is loading dynamic code: build fails with error: ```bash Failed to compile. ./middleware.js Dynamic Code Evaluation (e. g. 'eval', 'new Function', 'WebAssembly.compile') not allowed in Edge Runtime Used by default ./pages/api/route.js Dynamic Code Evaluation (e. g. 'eval', 'new Function', 'WebAssembly.compile') not allowed in Edge Runtime Used by default ``` ## Notes to reviewers Edge-related errors are either issued from `next/server/web/sandbox/context.ts` file (runtime errors) or from `next/build/webpack/plugins/middleware-plugin.ts` (webpack compilation). The previous implementation (I’m pleading guilty here) was way too verbose: some errors (Node.js global APIs like using `process.cwd()`) could be reported several times, and the previous mechanism to dedupe them (in middleware-plugin) wasn’t really effective. Changes in tests are due to renaming existing tests such as `test/integration/middleware-with-node.js-apis` into `test/integration/edge-runtime-with-node.js-apis`. I extended them to cover Edge API route. @hanneslund I’ve pushed the improvement you did in #38289 one step further to avoid duplication.
- Loading branch information
Showing
26 changed files
with
403 additions
and
470 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
8 changes: 8 additions & 0 deletions
8
...tion/middleware-dynamic-code/lib/utils.js → ...on/edge-runtime-dynamic-code/lib/utils.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
File renamed without changes.
21 changes: 13 additions & 8 deletions
21
...ion/middleware-dynamic-code/middleware.js → ...n/edge-runtime-dynamic-code/middleware.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,42 +1,47 @@ | ||
| import { notUsingEval, usingEval } from './lib/utils' | ||
| import { NextResponse } from 'next/server' | ||
| import { useCases, notUsingEval, usingEval } from './lib/utils' | ||
| import { | ||
| usingWebAssemblyCompile, | ||
| usingWebAssemblyInstantiate, | ||
| usingWebAssemblyInstantiateWithBuffer, | ||
| } from './lib/wasm' | ||
|
|
||
| export async function middleware(request) { | ||
| if (request.nextUrl.pathname === '/using-eval') { | ||
| if (request.nextUrl.pathname === `/${useCases.eval}`) { | ||
| return new Response(null, { | ||
| headers: { data: JSON.stringify(await usingEval()) }, | ||
| }) | ||
| } | ||
|
|
||
| if (request.nextUrl.pathname === '/not-using-eval') { | ||
| if (request.nextUrl.pathname === `/${useCases.noEval}`) { | ||
| return new Response(null, { | ||
| headers: { data: JSON.stringify(await notUsingEval()) }, | ||
| }) | ||
| } | ||
|
|
||
| if (request.nextUrl.pathname === '/using-webassembly-compile') { | ||
| if (request.nextUrl.pathname === `/${useCases.wasmCompile}`) { | ||
| return new Response(null, { | ||
| headers: { data: JSON.stringify(await usingWebAssemblyCompile(9)) }, | ||
| }) | ||
| } | ||
|
|
||
| if (request.nextUrl.pathname === '/using-webassembly-instantiate') { | ||
| if (request.nextUrl.pathname === `/${useCases.wasmInstanciate}`) { | ||
| return new Response(null, { | ||
| headers: { data: JSON.stringify(await usingWebAssemblyInstantiate(9)) }, | ||
| }) | ||
| } | ||
|
|
||
| if ( | ||
| request.nextUrl.pathname === '/using-webassembly-instantiate-with-buffer' | ||
| ) { | ||
| if (request.nextUrl.pathname === `/${useCases.wasmBufferInstanciate}`) { | ||
| return new Response(null, { | ||
| headers: { | ||
| data: JSON.stringify(await usingWebAssemblyInstantiateWithBuffer(9)), | ||
| }, | ||
| }) | ||
| } | ||
|
|
||
| return NextResponse.next() | ||
| } | ||
|
|
||
| export const config = { | ||
| matcher: Object.values(useCases).map((route) => `/${route}`), | ||
| } |
File renamed without changes.
26 changes: 26 additions & 0 deletions
26
test/integration/edge-runtime-dynamic-code/pages/api/route.js
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| import { useCases, notUsingEval, usingEval } from '../../lib/utils' | ||
| import { | ||
| usingWebAssemblyCompile, | ||
| usingWebAssemblyInstantiate, | ||
| usingWebAssemblyInstantiateWithBuffer, | ||
| } from '../../lib/wasm' | ||
|
|
||
| export default async function handler(request) { | ||
| const useCase = request.nextUrl.searchParams.get('case') | ||
|
|
||
| return Response.json( | ||
| useCase === useCases.eval | ||
| ? await usingEval() | ||
| : useCase === useCases.noEval | ||
| ? await notUsingEval() | ||
| : useCase === useCases.wasmCompile | ||
| ? await usingWebAssemblyCompile(9) | ||
| : useCase === useCases.wasmInstanciate | ||
| ? await usingWebAssemblyInstantiate(9) | ||
| : useCase === useCases.wasmBufferInstanciate | ||
| ? await usingWebAssemblyInstantiateWithBuffer(9) | ||
| : { ok: true } | ||
| ) | ||
| } | ||
|
|
||
| export const config = { runtime: 'experimental-edge' } |
File renamed without changes.
Oops, something went wrong.