Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .changeset/tame-pandas-render.md
Original file line number Diff line number Diff line change
@@ -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).
23 changes: 23 additions & 0 deletions examples/e2e/pages-router/e2e/ssr-not-found.test.ts
Original file line number Diff line number Diff line change
@@ -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("<!DOCTYPE html>");
expect(body).not.toBe("This page could not be found");
});
18 changes: 18 additions & 0 deletions examples/e2e/pages-router/src/pages/ssr-not-found/index.tsx
Original file line number Diff line number Diff line change
@@ -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<typeof getServerSideProps>) {
return null;
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
createCacheHandlerRule,
createComposableCacheHandlersRule,
disableNodeMiddlewareRule,
registerRouterServerContextRule,
} from "./next-server.js";

describe("Next Server", () => {
Expand Down Expand Up @@ -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(`
Expand Down
80 changes: 80 additions & 0 deletions packages/cloudflare/src/cli/build/patches/plugins/next-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,8 @@ export function patchNextServer(updater: ContentUpdater, buildOpts: BuildOptions

contents = patchCode(contents, attachRequestMetaRule);

contents = patchCode(contents, registerRouterServerContextRule);

return contents;
},
},
Expand Down Expand Up @@ -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
Expand Down
Loading