-
Notifications
You must be signed in to change notification settings - Fork 143
fix: render custom Pages Router 404 for getStaticProps/getServerSideProps notFound results #1346
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
Open
rome2o
wants to merge
2
commits into
opennextjs:main
Choose a base branch
from
rome2o:fix/pages-router-render404
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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,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). |
This file contains hidden or 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,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
18
examples/e2e/pages-router/src/pages/ssr-not-found/index.tsx
This file contains hidden or 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,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; | ||
| } |
This file contains hidden or 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 hidden or 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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
🔴 Query parameters for dynamic pages may be silently dropped on every request
The patch turns on Next.js's "hosted behind a router server" flag for the whole app (
isWrappedByNextServer = trueatpackages/cloudflare/src/cli/build/patches/plugins/next-server.ts:225) even though no router server exists here, so Next.js skips the step that turns internally-prefixed routing parameters back into real page parameters and instead deletes them.Impact: Pages that receive route parameters through internal prefixed query values can render with missing parameters, producing wrong or empty content instead of the expected page.
Why the registration flips a query-normalization branch that was previously never taken
Next reads the context in
RouteModule#prepare()using the keygetRequestMeta(req, 'relativeProjectDir') || this.relativeProjectDir(route-module.js:287-288 in next@15.5.21). Under OpenNext that key is"", while Next's own lazy self-registration inhandleCatchallRenderRequestwrites underpath.relative(process.cwd(), this.dir)— which the PR description itself observed to be".."at runtime. So before this patch the lookup at key""returnedundefinedand route-module.js:391 took theserverUtils.normalizeQueryParams(query, routeParamKeys)branch, which strips thenxtP/interception prefixes, decodes the values and re-adds them as real query keys, populatingrouteParamKeys.After this patch an entry now exists at key
""withisWrappedByNextServer = true, so the other branch runs:serverUtils.filterInternalQuery(query, []), which simply deletes everynxtP-/nxti-prefixed key and leavesrouteParamKeysempty. That branch is only correct when an upstream router server has already normalized the query, which is not the case for OpenNext.If only
render404is needed for the Pages RouternotFoundfix, registeringrender404(and optionallynextConfig) without settingisWrappedByNextServerkeeps the previous query handling intact.Prompt for agents
Was this helpful? React with 👍 or 👎 to provide feedback.
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 catch — fixed in 9ac06a9. Dropped the
isWrappedByNextServer = trueassignment; onlyrender404(andnextConfig, already read elsewhere inRouteModule) is needed for thenotFoundfix. Verified manually that/api/dynamic/[slug]still resolves params correctly with it removed, and the e2e regression test still passes.