-
Notifications
You must be signed in to change notification settings - Fork 328
fix(404): align default not-found copy with Next.js #1567
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| import DefaultNotFound from "vinext/shims/default-not-found"; | ||
|
|
||
| /** | ||
| * Module-shaped wrapper around vinext's built-in default not-found component. | ||
| * Used as the fallback when an app does not define its own `app/not-found.tsx` | ||
| * (and has not opted into `app/global-not-found.tsx`). The runtime treats any | ||
| * `{ default: Component }` record as a "not-found module", so wrapping the | ||
| * component this way lets us thread the default through the existing | ||
| * `rootNotFoundModule` plumbing without introducing a parallel code path. | ||
| * | ||
| * Mirrors Next.js's `defaultNotFoundPath` | ||
| * (`next/dist/client/components/builtin/not-found.js`), which is selected | ||
| * automatically when the user has not supplied a custom not-found file: | ||
| * https://github.com/vercel/next.js/blob/canary/packages/next/src/build/webpack/loaders/next-app-loader/index.ts | ||
| */ | ||
| export const DEFAULT_NOT_FOUND_MODULE = { | ||
| default: DefaultNotFound, | ||
| } as const; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,6 +45,7 @@ import { | |
| parseCookieLocaleFromHeader, | ||
| resolvePagesI18nRequest, | ||
| } from "./pages-i18n.js"; | ||
| import { buildDefaultPagesNotFoundResponse } from "./pages-default-404.js"; | ||
|
|
||
| /** | ||
| * Render a React element to a string using renderToReadableStream. | ||
|
|
@@ -1288,7 +1289,21 @@ async function renderErrorPage( | |
| } | ||
| } | ||
|
|
||
| // No custom error page found — use plain text fallback | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit (no action needed): The |
||
| // No custom error page found — fall back to vinext's default. The 404 case | ||
| // renders the canonical Next.js HTML body (matching `pages/_error.tsx`) so | ||
| // dev-server responses include "This page could not be found." just like | ||
| // production. Other status codes keep the plain-text fallback because | ||
| // Next.js's `_error.tsx` defaults already handle those cases when present. | ||
| if (statusCode === 404) { | ||
| const defaultResponse = buildDefaultPagesNotFoundResponse(); | ||
| const headers: Record<string, string> = {}; | ||
| defaultResponse.headers.forEach((value, key) => { | ||
| headers[key] = value; | ||
| }); | ||
| res.writeHead(defaultResponse.status, headers); | ||
| res.end(await defaultResponse.text()); | ||
| return; | ||
| } | ||
| res.writeHead(statusCode, { "Content-Type": "text/plain" }); | ||
| res.end(`${statusCode} - ${statusCode === 404 ? "Page not found" : "Internal Server Error"}`); | ||
| res.end(`${statusCode} - Internal Server Error`); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| /** | ||
| * Default 404 HTML body for the Pages Router. | ||
| * | ||
| * Used when a Pages Router request does not match any route (and the app has | ||
| * not supplied a custom `pages/404.tsx`). Mirrors the markup Next.js's | ||
| * `pages/_error.tsx` produces for a 404 response: a centered status / message | ||
| * pair plus minified theme CSS and dark-mode media query. The message string | ||
| * `"This page could not be found."` (note the trailing period) is the | ||
| * canonical body asserted by Next.js's deploy suite | ||
| * (`test/e2e/getserversideprops/test/index.test.ts`, | ||
| * `test/e2e/basepath/error-pages.test.ts`). | ||
| * | ||
| * Kept as a hand-rendered HTML literal rather than a React-rendered template | ||
| * because the Pages Router server entry is invoked from both Workers and the | ||
| * dev server before any React-renderer wiring is available for this path — | ||
| * matching the lightweight build-time strategy Next.js uses for its packaged | ||
| * `_error` static fallback. See: | ||
| * .nextjs-ref/packages/next/src/pages/_error.tsx | ||
| */ | ||
|
|
||
| const STATUS = 404; | ||
| const MESSAGE = "This page could not be found."; | ||
|
|
||
| const CSS = `body{color:#000;background:#fff;margin:0}.next-error-h1{border-right:1px solid rgba(0,0,0,.3)}@media (prefers-color-scheme:dark){body{color:#fff;background:#000}.next-error-h1{border-right:1px solid rgba(255,255,255,.3)}}`; | ||
|
|
||
| const HTML = `<!DOCTYPE html><html><head><meta charSet="utf-8"/><meta name="viewport" content="width=device-width"/><title>${STATUS}: ${MESSAGE}</title><meta name="next-head-count" content="2"/><style data-next-hide-fouc="true">body{display:none}</style><noscript data-next-hide-fouc="true"><style>body{display:block}</style></noscript></head><body><div id="__next"><div style="font-family:system-ui,"Segoe UI",Roboto,Helvetica,Arial,sans-serif,"Apple Color Emoji","Segoe UI Emoji";height:100vh;text-align:center;display:flex;flex-direction:column;align-items:center;justify-content:center"><div style="line-height:48px"><style>${CSS}</style><h1 class="next-error-h1" style="display:inline-block;margin:0 20px 0 0;padding-right:23px;font-size:24px;font-weight:500;vertical-align:top;line-height:49px">${STATUS}</h1><div style="display:inline-block"><h2 style="font-size:14px;font-weight:400;line-height:28px">${MESSAGE}</h2></div></div></div></div></body></html>`; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug:
Browsers are forgiving here (they handle it case-insensitively), so this won't cause user-visible breakage, but it's technically malformed HTML. Since the goal is to match Next.js's Bug: stray Next.js's For reference, Next.js's h1: {
display: 'inline-block',
margin: '0 20px 0 0',
paddingRight: 23,
fontSize: 24,
fontWeight: 500,
verticalAlign: 'top',
// NO lineHeight here
}Fix both on this line: |
||
|
|
||
| /** | ||
| * Build the Next.js-compatible default 404 HTML response for the Pages Router. | ||
| * Content-type is `text/html; charset=utf-8`, matching Next.js's | ||
| * `pages-handler` 404 response. | ||
| */ | ||
| export function buildDefaultPagesNotFoundResponse(): Response { | ||
| return new Response(HTML, { | ||
| status: STATUS, | ||
| headers: { "Content-Type": "text/html; charset=utf-8" }, | ||
| }); | ||
| } | ||
|
|
||
| /** Exported for tests / callers that need the raw HTML body. */ | ||
| export const DEFAULT_PAGES_NOT_FOUND_HTML = HTML; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,90 @@ | ||
| /** | ||
| * Ported from Next.js's built-in default not-found component: | ||
| * https://github.com/vercel/next.js/blob/canary/packages/next/src/client/components/builtin/not-found.tsx | ||
| * https://github.com/vercel/next.js/blob/canary/packages/next/src/client/components/http-access-fallback/error-fallback.tsx | ||
| * | ||
| * Rendered when an App Router request resolves to a 404 and the user has not | ||
| * supplied their own `app/not-found.tsx` (or `app/global-not-found.tsx`). | ||
| * Matches Next.js's `HTTPAccessErrorFallback` exactly: a centered 404 / message | ||
| * pair with minified theme CSS and dark-mode media query. | ||
| * | ||
| * The message string `"This page could not be found."` (note the trailing | ||
| * period) is the canonical body asserted by Next.js's deploy suite | ||
| * (`test/e2e/app-dir/prefetching-not-found/prefetching-not-found.test.ts`, | ||
| * `test/e2e/basepath/error-pages.test.ts`). | ||
| */ | ||
| import React from "react"; | ||
|
|
||
| const styles = { | ||
| error: { | ||
| fontFamily: | ||
| 'system-ui,"Segoe UI",Roboto,Helvetica,Arial,sans-serif,"Apple Color Emoji","Segoe UI Emoji"', | ||
| height: "100vh", | ||
| textAlign: "center" as const, | ||
| display: "flex", | ||
| flexDirection: "column" as const, | ||
| alignItems: "center", | ||
| justifyContent: "center", | ||
| }, | ||
| desc: { | ||
| display: "inline-block", | ||
| }, | ||
| h1: { | ||
| display: "inline-block", | ||
| margin: "0 20px 0 0", | ||
| padding: "0 23px 0 0", | ||
| fontSize: 24, | ||
| fontWeight: 500, | ||
| verticalAlign: "top", | ||
| lineHeight: "49px", | ||
| }, | ||
| h2: { | ||
| fontSize: 14, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. App Router component looks correct — styles match Next.js's |
||
| fontWeight: 400, | ||
| lineHeight: "49px", | ||
| margin: 0, | ||
| }, | ||
| } satisfies Record<string, React.CSSProperties>; | ||
|
|
||
| const STATUS = 404; | ||
| const MESSAGE = "This page could not be found."; | ||
|
|
||
| /** | ||
| * Mirrors `<HTTPAccessErrorFallback status={404} message="This page could not be found." />` | ||
| * from Next.js. Kept in sync with the upstream component's structure so HTML | ||
| * snapshot diffs between Next.js and vinext stay minimal. | ||
| */ | ||
| export default function DefaultNotFound(): React.ReactElement { | ||
| return React.createElement( | ||
| React.Fragment, | ||
| null, | ||
| React.createElement("title", null, `${STATUS}: ${MESSAGE}`), | ||
| React.createElement( | ||
| "div", | ||
| { style: styles.error }, | ||
| React.createElement( | ||
| "div", | ||
| null, | ||
| React.createElement("style", { | ||
| dangerouslySetInnerHTML: { | ||
| __html: | ||
| "body{color:#000;background:#fff;margin:0}.next-error-h1{border-right:1px solid rgba(0,0,0,.3)}@media (prefers-color-scheme:dark){body{color:#fff;background:#000}.next-error-h1{border-right:1px solid rgba(255,255,255,.3)}}", | ||
| }, | ||
| }), | ||
| React.createElement( | ||
| "h1", | ||
| { | ||
| className: "next-error-h1", | ||
| style: styles.h1, | ||
| }, | ||
| STATUS, | ||
| ), | ||
| React.createElement( | ||
| "div", | ||
| { style: styles.desc }, | ||
| React.createElement("h2", { style: styles.h2 }, MESSAGE), | ||
| ), | ||
| ), | ||
| ), | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -491,6 +491,48 @@ describe("app fallback renderer default global error UI", () => { | |
| }); | ||
| }); | ||
|
|
||
| // Regression for #1454 — default App Router 404 must match Next.js's built-in | ||
| // not-found component ("This page could not be found." with trailing period). | ||
| describe("app fallback renderer default not-found UI", () => { | ||
| it("renders the canonical 'This page could not be found.' body when no not-found.tsx exists", async () => { | ||
| const { renderer } = createRenderer(); | ||
| const request = new Request("https://example.com/missing"); | ||
|
|
||
| const response = await renderer.renderNotFound(null, false, request, undefined, undefined, { | ||
| headers: null, | ||
| status: null, | ||
| }); | ||
|
|
||
| expect(response?.status).toBe(404); | ||
| const html = await response?.text(); | ||
| // Canonical message must contain the trailing period to match Next.js | ||
| // (see .nextjs-ref/packages/next/src/client/components/builtin/not-found.tsx). | ||
| expect(html).toContain("This page could not be found."); | ||
| // Status code is surfaced as the <h1>. | ||
| expect(html).toContain("404"); | ||
| // Old vinext default ("404 - Page not found") must NOT leak through. | ||
| expect(html).not.toContain("404 - Page not found"); | ||
| }); | ||
|
|
||
| it("prefers a user-defined root not-found.tsx over the default", async () => { | ||
| const { renderer } = createRenderer({ rootNotFoundModule: notFoundModule }); | ||
| const request = new Request("https://example.com/missing"); | ||
|
|
||
| const response = await renderer.renderNotFound(null, false, request, undefined, undefined, { | ||
| headers: null, | ||
| status: null, | ||
| }); | ||
|
|
||
| expect(response?.status).toBe(404); | ||
| const html = await response?.text(); | ||
| // The user-defined boundary wins. | ||
| expect(html).toContain('data-boundary="not-found"'); | ||
| expect(html).toContain("Missing page"); | ||
| // The default not-found body must NOT leak through. | ||
| expect(html).not.toContain("This page could not be found."); | ||
| }); | ||
| }); | ||
|
|
||
| // Mirrors Next.js 16 experimental.globalNotFound behavior. | ||
| // Ported from Next.js: test/e2e/app-dir/global-not-found/{basic,both-present,not-present}. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nice behavioral change. This test previously asserted |
||
| // Source: https://github.com/vercel/next.js/blob/canary/test/e2e/app-dir/global-not-found | ||
|
|
@@ -581,8 +623,9 @@ describe("app fallback renderer with globalNotFoundModule", () => { | |
| // Mirrors test/e2e/app-dir/global-not-found/not-present: when the user | ||
| // opted into experimental.globalNotFound but never created the file, | ||
| // route-miss 404s should still serve the default 404 response. With no | ||
| // root notFoundModule either, the renderer returns null and the caller | ||
| // falls back to the framework's 404 Response. | ||
| // user-defined root notFoundModule either, vinext renders its built-in | ||
| // default not-found component (parity with Next.js's packaged | ||
| // not-found.tsx — "This page could not be found." with trailing period). | ||
| const { renderer } = createRenderer({ | ||
| globalNotFoundModule: null, | ||
| rootLayoutModules: [rootLayoutModule], | ||
|
|
@@ -594,8 +637,9 @@ describe("app fallback renderer with globalNotFoundModule", () => { | |
| status: null, | ||
| }); | ||
|
|
||
| // No boundary component to render -> renderer returns null. | ||
| expect(response).toBeNull(); | ||
| expect(response?.status).toBe(404); | ||
| const html = await response?.text(); | ||
| expect(html).toContain("This page could not be found."); | ||
| }); | ||
|
|
||
| it("does not use global-not-found for non-404 access fallbacks (403, 401)", async () => { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| /** | ||
| * Regression for #1454. | ||
| * | ||
| * The Pages Router default 404 response (used when no `pages/404.tsx` and no | ||
| * `pages/_error.tsx` is defined) must include the canonical Next.js body | ||
| * `"This page could not be found."` (with trailing period). Pre-fix vinext | ||
| * shipped a minimal `<h1>404 - Page not found</h1>` placeholder that broke | ||
| * deploy-suite parity against `test/e2e/getserversideprops/test/index.test.ts` | ||
| * and `test/e2e/basepath/error-pages.test.ts`. | ||
| */ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { | ||
| buildDefaultPagesNotFoundResponse, | ||
| DEFAULT_PAGES_NOT_FOUND_HTML, | ||
| } from "../packages/vinext/src/server/pages-default-404.js"; | ||
|
|
||
| describe("buildDefaultPagesNotFoundResponse", () => { | ||
| it("returns a 404 status with the canonical Next.js body", async () => { | ||
| const response = buildDefaultPagesNotFoundResponse(); | ||
| expect(response.status).toBe(404); | ||
| const body = await response.text(); | ||
| // The Next.js deploy suite asserts on this substring (with the trailing | ||
| // period — see test/e2e/basepath/error-pages.test.ts). | ||
| expect(body).toContain("This page could not be found."); | ||
| // The 404 status code is rendered in the heading. | ||
| expect(body).toContain("404"); | ||
| // Old vinext placeholder body must NOT leak through. | ||
| expect(body).not.toContain("404 - Page not found"); | ||
| }); | ||
|
|
||
| it("uses Next.js-compatible content-type", () => { | ||
| const response = buildDefaultPagesNotFoundResponse(); | ||
| expect(response.headers.get("Content-Type")).toBe("text/html; charset=utf-8"); | ||
| }); | ||
|
|
||
| it("exposes the raw HTML body for callers that need it", () => { | ||
| expect(DEFAULT_PAGES_NOT_FOUND_HTML).toContain("This page could not be found."); | ||
| expect(DEFAULT_PAGES_NOT_FOUND_HTML).toContain("<!DOCTYPE html>"); | ||
| }); | ||
| }); |
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.
Clean. Mirrors the
effectiveGlobalErrorModulepattern perfectly. Theas unknown as TModulecast is unavoidable given the generic signature — same trade-off as the global error fallback.