diff --git a/.changeset/tame-pandas-render.md b/.changeset/tame-pandas-render.md new file mode 100644 index 000000000..afe0111b3 --- /dev/null +++ b/.changeset/tame-pandas-render.md @@ -0,0 +1,16 @@ +--- +"@opennextjs/cloudflare": patch +--- + +fix: render the app's custom 404 page for Pages Router `notFound: true` results + +Register Next.js's `routerServerContext` (which provides `render404`) unconditionally +before the first request is handled, instead of relying on Next.js's own lazy +self-registration inside `handleCatchallRenderRequest`. + +Previously, when a Pages Router page's `getStaticProps`/`getServerSideProps` returned +`{ notFound: true }`, `routerServerContext.render404` was undefined for any request that +matched a real page (as opposed to a genuinely unknown path), so Next.js fell back to the +bare hardcoded `"This page could not be found"` body instead of rendering the app's actual +`pages/404`/`pages/_error`. This mirrors the same class of bug fixed for Pages Router in +Cloudflare's `vinext` project (cloudflare/vinext#1737, cloudflare/vinext#2773). diff --git a/examples/e2e/pages-router/e2e/ssr-not-found.test.ts b/examples/e2e/pages-router/e2e/ssr-not-found.test.ts new file mode 100644 index 000000000..248a45bea --- /dev/null +++ b/examples/e2e/pages-router/e2e/ssr-not-found.test.ts @@ -0,0 +1,23 @@ +import { expect, test } from "@playwright/test"; + +test("should render the app's 404 page for a getServerSideProps `notFound` result, not a bare fallback body", async ({ + page, +}) => { + // `/ssr-not-found` always returns `{ notFound: true }` from `getServerSideProps`, which runs on + // every request (unlike a `getStaticProps` page with `fallback: false`, whose `notFound` paths are + // resolved at build time). This means it can be the very first request a fresh Worker isolate + // handles - unlike a route that matches no page at all, which goes through Next's catch-all + // handling. If the router server context (and its `render404`) isn't registered before that first + // request, Next.js falls back to a bare, unstyled `"This page could not be found"` string instead + // of actually rendering the app's 404/error page - see next-server.ts's + // `registerRouterServerContextRule`. + const result = await page.goto("/ssr-not-found"); + expect(result).toBeDefined(); + expect(result?.status()).toBe(404); + + const body = await result?.text(); + // The bare fallback body is the literal, unwrapped string "This page could not be found" with no + // HTML document around it. A real render produces a full HTML document. + expect(body).toContain(""); + expect(body).not.toBe("This page could not be found"); +}); diff --git a/examples/e2e/pages-router/src/pages/ssr-not-found/index.tsx b/examples/e2e/pages-router/src/pages/ssr-not-found/index.tsx new file mode 100644 index 000000000..d4e0928f7 --- /dev/null +++ b/examples/e2e/pages-router/src/pages/ssr-not-found/index.tsx @@ -0,0 +1,18 @@ +import type { InferGetServerSidePropsType } from "next"; + +/** + * `getServerSideProps` runs on every request (unlike `getStaticProps` with `fallback: false`, + * which resolves `notFound` at build time). This makes it possible for this route to be the + * very first request handled by a fresh Worker isolate, which is what regresses if + * `routerServerContext.render404` isn't registered before the first request. + * + * See e2e/ssr-not-found.test.ts's "should render the app's 404 page for a getServerSideProps + * `notFound` result, not a bare fallback body" test. + */ +export async function getServerSideProps() { + return { notFound: true }; +} + +export default function Page({}: InferGetServerSidePropsType) { + return null; +} diff --git a/packages/cloudflare/src/cli/build/patches/plugins/next-server.spec.ts b/packages/cloudflare/src/cli/build/patches/plugins/next-server.spec.ts index a86594666..793d6bff4 100644 --- a/packages/cloudflare/src/cli/build/patches/plugins/next-server.spec.ts +++ b/packages/cloudflare/src/cli/build/patches/plugins/next-server.spec.ts @@ -7,6 +7,7 @@ import { createCacheHandlerRule, createComposableCacheHandlersRule, disableNodeMiddlewareRule, + registerRouterServerContextRule, } from "./next-server.js"; describe("Next Server", () => { @@ -363,6 +364,74 @@ class NextNodeServer extends _baseserver.default { `); }); + // Note: the leading single-line comments before `this.prepare().catch(...)` (as found in real + // Next.js 16.2.11 builds) are intentional - a previous version of `registerRouterServerContextRule` + // captured/reprinted the whole method body via a `$$$BODY` meta-variable, which collapsed these + // comments onto a single line and turned the rest of the method into a comment, corrupting it. + const makeRequestHandlerCode = ` +class NextNodeServer extends _baseserver.default { + // ... + makeRequestHandler() { + // This is just optimization to fire prepare as soon as possible. It will be + // properly awaited later. We add the catch here to ensure that it does not + // cause an unhandled promise rejection. The promise rejection will be + // handled later on via the \`await\` when the request handler is called. + this.prepare().catch((err)=>{ + console.error('Failed to prepare server', err); + }); + const handler = super.getRequestHandler(); + return (req, res, parsedUrl)=>handler(this.normalizeReq(req), this.normalizeRes(res), parsedUrl); + } + // ... +} +`; + + test("register router server context", () => { + expect(computePatchDiff("next-server.js", makeRequestHandlerCode, registerRouterServerContextRule)) + .toMatchInlineSnapshot(` + "Index: next-server.js + =================================================================== + --- next-server.js + +++ next-server.js + @@ -1,5 +1,4 @@ + - + class NextNodeServer extends _baseserver.default { + // ... + makeRequestHandler() { + // This is just optimization to fire prepare as soon as possible. It will be + @@ -9,8 +8,27 @@ + this.prepare().catch((err)=>{ + console.error('Failed to prepare server', err); + }); + const handler = super.getRequestHandler(); + - return (req, res, parsedUrl)=>handler(this.normalizeReq(req), this.normalizeRes(res), parsedUrl); + + if (!_routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol]) { + + _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol] = {}; + +} + +// Note: this is hardcoded to "" rather than computed via \`_path.relative(process.cwd(), this.dir)\` + +// (which is what Next.js's own self-registration in \`handleCatchallRenderRequest\` does) because + +// \`process.cwd()\` at request-handling time is not guaranteed to match \`process.cwd()\` at + +// \`NextNodeServer\` construction time in this runtime (observed to differ by one directory level, + +// e.g. yielding ".." here). The read side, \`RouteModule#getRouterServerContext\`, falls back to + +// \`this.relativeProjectDir\`, which every route module in the OpenNext build has hardcoded to "" + +// (OpenNext always constructs \`NextServer\` with \`dir: ""\`). Using "" here keeps the write side in + +// sync with that build-time constant instead of a runtime-computed value that can drift from it. + +const relativeProjectDir = ""; + +const existingServerContext = _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir]; + +if (!existingServerContext) { + + _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir] = { + + render404: this.render404.bind(this) + + }; + +} + +_routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir].nextConfig = this.nextConfig; + +return (req, res, parsedUrl)=>handler(this.normalizeReq(req), this.normalizeRes(res), parsedUrl); + } + // ... + } + " + `); + }); + test("attachRequestMeta", () => { expect(computePatchDiff("next-server.js", next15ServerCode, attachRequestMetaRule)) .toMatchInlineSnapshot(` diff --git a/packages/cloudflare/src/cli/build/patches/plugins/next-server.ts b/packages/cloudflare/src/cli/build/patches/plugins/next-server.ts index 15e27b4d4..4aa41375f 100644 --- a/packages/cloudflare/src/cli/build/patches/plugins/next-server.ts +++ b/packages/cloudflare/src/cli/build/patches/plugins/next-server.ts @@ -44,6 +44,8 @@ export function patchNextServer(updater: ContentUpdater, buildOpts: BuildOptions contents = patchCode(contents, attachRequestMetaRule); + contents = patchCode(contents, registerRouterServerContextRule); + return contents; }, }, @@ -145,6 +147,84 @@ fix: |- * Callstack: handleRequest-> handleRequestImpl -> attachRequestMeta * */ +/** + * Registers a `routerServerContext` (with a working `render404`) before any request is handled, + * instead of relying on Next.js's own lazy self-registration. + * + * Next.js's Pages Router falls back to a bare, hardcoded `"This page could not be found"` body + * (see `next/dist/server/route-modules/pages/pages-handler.js`) whenever a page's + * `getStaticProps`/`getServerSideProps` returns `{ notFound: true }` and no `routerServerContext.render404` + * is available - instead of rendering the app's actual `pages/404`/`pages/_error`. + * + * `routerServerContext` is read from a well-known global registry + * (`routerServerGlobal[RouterServerContextSymbol][relativeProjectDir]`, see + * `next/dist/server/lib/router-utils/router-server-context.js`) that a real `next start` router-server + * process populates upfront. `NextNodeServer` (`next/dist/server/next-server.js`) *does* also + * self-register into that same registry, but only lazily, inside `handleCatchallRenderRequest` + * (i.e. only once an unmatched/catch-all path is hit). + * + * OpenNext never runs a router-server process and constructs a bare `NextServer` directly + * (see `@opennextjs/aws`'s `dist/core/util.js`), calling `getRequestHandler()`/`makeRequestHandler()` + * once at startup. Any request that matches a real page - i.e. it never goes through + * `handleCatchallRenderRequest` - can therefore be the very first request handled in a Worker + * isolate, before Next.js's lazy self-registration has ever run. If that page's data method returns + * `notFound: true`, `routerServerContext` is `undefined` in `RouteModule#prepare()`, `render404` is + * unavailable, and Next.js falls back to the bare hardcoded body instead of the designed 404 page. + * + * We fix this by performing the same self-registration Next.js already does in + * `handleCatchallRenderRequest` (reusing `this.render404`, which correctly renders `pages/404`/ + * `pages/_error` - it's the same method that already powers 404s for genuinely unmatched paths), + * but unconditionally in `makeRequestHandler()`, which always runs before the request handler is + * returned and therefore before any request - matched or not - can reach the route module. + * + * Prior art: this mirrors the fix Cloudflare's `vinext` project shipped for the equivalent Pages + * Router bug - rerouting `notFound` results to the app's actual 404/error page instead of a + * built-in fallback - see https://github.com/cloudflare/vinext/pull/1737 and + * https://github.com/cloudflare/vinext/pull/2773 (header preservation follow-up). + */ +// Note: this deliberately does NOT capture the whole `makeRequestHandler` body via a `$$$BODY` +// meta-variable (e.g. `context: "class { makeRequestHandler() { $$$BODY } }"`). Doing so requires +// ast-grep to re-serialize the captured statements, and at least in Next.js 16.2.11's +// `next-server.js`, `makeRequestHandler`'s first statement (`this.prepare().catch(...)`) is preceded +// by several consecutive single-line (`//`) comments. Re-serializing them collapses them onto one +// line, which turns everything after the first `//` - including `this.prepare().catch(...)` itself - +// into a comment, corrupting the method and breaking the build with a syntax error. +// +// Matching only the `return (req, res, parsedUrl) => ...` statement and inserting before it avoids +// capturing/reprinting any of the method's other statements (and their leading comments) entirely. +export const registerRouterServerContextRule = ` +rule: + kind: return_statement + pattern: return $EXPR; + inside: + kind: method_definition + has: + field: name + regex: ^makeRequestHandler$ + stopBy: end +fix: |- + if (!_routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol]) { + _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol] = {}; + } + // Note: this is hardcoded to "" rather than computed via \`_path.relative(process.cwd(), this.dir)\` + // (which is what Next.js's own self-registration in \`handleCatchallRenderRequest\` does) because + // \`process.cwd()\` at request-handling time is not guaranteed to match \`process.cwd()\` at + // \`NextNodeServer\` construction time in this runtime (observed to differ by one directory level, + // e.g. yielding ".." here). The read side, \`RouteModule#getRouterServerContext\`, falls back to + // \`this.relativeProjectDir\`, which every route module in the OpenNext build has hardcoded to "" + // (OpenNext always constructs \`NextServer\` with \`dir: ""\`). Using "" here keeps the write side in + // sync with that build-time constant instead of a runtime-computed value that can drift from it. + const relativeProjectDir = ""; + const existingServerContext = _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir]; + if (!existingServerContext) { + _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir] = { + render404: this.render404.bind(this) + }; + } + _routerservercontext.routerServerGlobal[_routerservercontext.RouterServerContextSymbol][relativeProjectDir].nextConfig = this.nextConfig; + return $EXPR; +`; + export const attachRequestMetaRule = ` rule: kind: identifier