fix(pages): render custom errors for notFound results - #1737
Conversation
Pages getStaticProps and getServerSideProps notFound HTML results returned the built-in 404 response directly. That skipped the configured pages/404 or pages/_error render path, so _error.getInitialProps never received ctx.req.url or ctx.asPath for the original request. The page-data resolver now returns a typed notFound signal for HTML requests while preserving JSON-shaped 404 data responses. The generated Pages entry rerenders the configured not-found route with the original asPath, and Pages getInitialProps is loaded through a shared typed helper for prod and dev parity.
commit: |
…r context, and response-sent handling - Call getInitialProps with component as this receiver - Thread caught errors through to _error.getInitialProps(ctx.err) - Check headersSent || writableEnded in dev server after getInitialProps - Add tests: gSSP notFound, explicit 404, data JSON 404, err on throw, this binding
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: render custom errors for notFound results
Solid, well-scoped change that brings Pages Router notFound handling closer to Next.js. The core design — having resolvePagesPageData() emit a typed notFound signal for HTML while preserving the JSON 404 envelope for /_next/data requests, and letting the generated entry own the /404 -> /_error fallback — matches the upstream render404() flow. The recursion guard (routePattern !== "/404" && routePattern !== "/_error") correctly prevents infinite loops, and err is forwarded only on the SSR-throw path (not the notFound path), which is the right distinction.
I ran the targeted suites locally and all pass:
vp test run tests/pages-page-data.test.ts(23 passed)vp test run tests/pages-router.test.ts(new notFound + gssp-throw tests pass)vp test run tests/entry-templates.test.ts(24 passed)vp checkon the changed server files (clean)
A few non-blocking observations below — none of them block merge.
Observations
1. _app.getInitialProps interaction (pre-existing gap, not introduced here). The new block calls the page component's getInitialProps directly, regardless of whether a custom _app defines getInitialProps. In Next.js, when _app has getInitialProps, the App's gIP owns calling the page's gIP (loadGetInitialProps(ctx.Component, ctx.ctx)) and wraps the result as { pageProps }. vinext doesn't appear to invoke App.getInitialProps anywhere today, so this PR doesn't regress that — it improves the common (no custom App gIP) case. Worth a tracking issue if not already filed.
2. Stricter-than-Next.js return validation. The helper rejects non-plain-object returns (arrays, functions) via isPropsObject, whereas Next.js loadGetInitialProps only rejects falsy values (if (!props)). This is arguably better, but it is a deliberate divergence from upstream — flagging in case strict parity is preferred here.
3. Minor: response-sent check asymmetry. The prod block in pages-page-data.ts checks only res.headersSent while the dev block and the helper check headersSent || writableEnded. This is correct in practice because the prod PagesReqResResponse backs headersSent with its ended flag (so res.end() flips it), but the inline check reads slightly inconsistently next to the helper. See inline comment.
Nice test coverage — the upstream fixture port plus the lower-boundary pages-page-data contract tests give good confidence.
| defaultLocale: options.i18n.defaultLocale, | ||
| }); | ||
|
|
||
| if (res.headersSent) { |
There was a problem hiding this comment.
Minor: the helper's isResponseSent checks headersSent || writableEnded, and the dev-server block checks both, but here only res.headersSent is checked. It's correct for the prod PagesReqResResponse (its headersSent getter is backed by the ended flag set in end()), so there's no actual bug — but the asymmetry reads oddly next to the helper. Consider relying on the helper's own sent-detection or mirroring the dev check for consistency.
|
Review posted successfully to PR #1737. SummaryI reviewed PR #1737 ( Verdict: Solid, well-scoped change. No blocking issues found. What's good:
Verification performed locally (all pass):
Non-blocking observations raised:
|
…r sent-check consistency The prod block in resolvePagesPageData checked only res.headersSent, while the helper's isResponseSent and the dev server check both headersSent and writableEnded. The prod PagesReqResResponse only declares headersSent (its getter is backed by the ended flag), so this is functionally correct in prod, but the asymmetry is hard to read next to the helper and a future change to the prod response shape would silently break sent-detection. Export isResponseSent from pages-get-initial-props and use it at both gSSP and gIP call sites so the prod resolver reuses the same sent-detection logic as the helper. Also adds tests for the writableEnded-only path and a focused unit test for isResponseSent covering both prod-shaped and node-shaped res mocks.
|
Thanks for the review! Addressing the three observations: 1. Confirmed not introduced by this PR. Searching for
This is the pre-existing behavior — the page gIP path works, the App gIP path is a known gap unrelated to notFound handling. I agree a tracking issue is worth filing (will do separately) but fixing it here would expand the PR's scope significantly (requires App gIP loading + 2. Stricter-than-Next.js return validation (deliberate divergence) — no change The 3. Response-sent check asymmetry — fixed Implemented in Added tests:
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: render custom errors for notFound results
Well-scoped, correct change. I re-ran the targeted suites locally and everything passes:
tests/pages-page-data.test.ts— 26 passedtests/pages-router.test.ts(the three new notFound + gssp-throw tests) — passedtests/entry-templates.test.ts— 24 passedvp checkon the three changed server files — clean
The core design matches Next.js's render404() flow: resolvePagesPageData() emits a typed notFound signal for HTML while preserving the JSON 404 envelope for /_next/data, and the generated entry owns the /404 → /_error fallback. I verified against upstream loadGetInitialProps (packages/next/src/shared/lib/utils.ts) — the this-binding, the sent-response short-circuit, and the asPath/req.url propagation all line up. The recursion guard (routePattern !== "/404" && routePattern !== "/_error") is correct, err is forwarded only on the SSR-throw path (not the notFound recursion), and __findPagesNotFoundRoute() is strictly better than the old matchRoute("/404") since it can't accidentally match a catch-all.
The three follow-ups from the prior review are resolved appropriately (the isResponseSent unification is a nice future-proofing). One new non-blocking perf observation below.
Non-blocking observations
1. createGsspReqRes() now runs on every static page without gSP/gSSP, even those with no getInitialProps. See inline comment — the req/res pair (header iteration + an unresolved responsePromise) is allocated per request on the static-page hot path, then thrown away because loadPagesGetInitialProps returns null early when there's no gIP. Worth gating behind a cheap gIP-presence check.
2. Sent-response branch diverges slightly from upstream. Next.js's loadGetInitialProps returns props unchanged when isResSent(res) is true (even if falsy/non-object). The helper normalizes non-objects to {} in that branch (pages-get-initial-props.ts:76). This is defensible (the caller spreads the result into pageProps, so a non-object would crash), just flagging the intentional difference.
3. Missing the dev-only instance-method guard. Upstream loadGetInitialProps throws in dev if getInitialProps is defined as a prototype/instance method (App.prototype?.getInitialProps). The helper omits this. Minor dev-ergonomics gap, not worth holding the PR.
Nice test coverage — the upstream fixture port plus the lower-boundary pages-page-data contract tests give good confidence.
| typeof options.pageModule.getServerSideProps !== "function" && | ||
| typeof options.pageModule.getStaticProps !== "function" | ||
| ) { | ||
| const { req, res, responsePromise } = options.createGsspReqRes(); |
There was a problem hiding this comment.
createGsspReqRes() is invoked unconditionally for every static page that lacks getServerSideProps/getStaticProps — i.e. the common case of a plain static page with no getInitialProps either. createPagesReqRes iterates request headers, allocates the req/res shims, and creates an unresolved responsePromise (pages-node-compat.ts:128-194), all of which is discarded the moment loadPagesGetInitialProps returns null because there's no gIP function.
Consider checking for a getInitialProps function before building the req/res. That keeps the hot path for ordinary static pages allocation-free and avoids leaving a dangling unresolved promise per request. For example, export a small hasPagesGetInitialProps(component) (or reuse the existing private getInitialPropsFn) from pages-get-initial-props.ts and gate on it:
| const { req, res, responsePromise } = options.createGsspReqRes(); | |
| if ( | |
| typeof options.pageModule.getServerSideProps !== "function" && | |
| typeof options.pageModule.getStaticProps !== "function" && | |
| hasPagesGetInitialProps(options.pageModule.default) | |
| ) { | |
| const { req, res, responsePromise } = options.createGsspReqRes(); |
(The same applies to the dev-server block at dev-server.ts:1057 and the renderErrorPage call at dev-server.ts:1510, though those are less hot.)
There was a problem hiding this comment.
Addressed in 81badc9. Exported hasPagesGetInitialProps from pages-get-initial-props.ts and gated the prod block on it. The hot path for plain static pages (no gSSP, no gSP, no gIP) is now allocation-free.
|
Review posted successfully to PR #1737. SummaryI reviewed PR #1737 ( Verdict: Solid, correct, well-scoped change. No blocking issues. Verification performed locally (all pass):
What I confirmed against upstream Next.js (
New observation raised (inline, non-blocking):
The three prior-review follow-ups were resolved appropriately. |
…check The getInitialProps prod block in resolvePagesPageData unconditionally called createGsspReqRes() for every static page without gSP/gSSP, even when the page had no getInitialProps either. This allocated a req/res pair and an unresolved responsePromise that was immediately discarded. Export hasPagesGetInitialProps from pages-get-initial-props.ts and gate the prod block on it, keeping the hot path for plain static pages allocation-free.
|
Addressed the three observations from the latest review: 1. 2. Sent-response normalizes non-objects to 3. Missing dev-only instance-method guard — no change. Minor dev-ergonomics improvement beyond the scope of this PR. |
|
/bigbonk review |
There was a problem hiding this comment.
Re-review: render custom errors for notFound results
Verified the latest revision (81badc98). This is a clean, well-scoped change and the three prior-review follow-ups are resolved appropriately. No blocking issues.
Verification (all green locally):
vp test run tests/pages-page-data.test.ts— 26 passedvp test run tests/pages-router.test.ts— the 3 new notFound + gssp-throw integration tests pass (_error.getInitialPropsreqUrl/asPath, explicit/404over/_error, gSSP-throwerrpropagation)vp test run tests/entry-templates.test.ts— 24 passedvp checkon the 3 changed server files — clean (format, lint, types)
What I confirmed:
- Core design mirrors Next.js
render404():resolvePagesPageData()emits a typednotFoundsignal for HTML, JSON 404 envelope preserved for/_next/data, and the generated entry owns the/404→/_errorfallback. __findPagesNotFoundRoute()is strictly safer than the oldmatchRoute("/404")(can't match a catch-all), and the recursion guard (routePattern !== "/404" && routePattern !== "/_error") prevents infinite loops on a custom 404/_errorpage that itself returnsnotFound.erris forwarded only on the SSR-throw path (entry L1048), not on the notFound recursion (L833) — correct distinction.req.url/asPathflow into_error.getInitialPropsvia the recursion's__forcedRoute+routeUrl.- The new
hasPagesGetInitialPropsgate (pages-page-data.ts:687) keeps the plain-static-page hot path allocation-free, addressing the prior perf observation. loadPagesGetInitialPropsthis-binding (getInitialProps.call(component, context)) and sent-response short-circuit match upstreamloadGetInitialProps.- prod-server.ts delegates Pages rendering to the built entry, so it inherits the new logic — no separate parity update needed.
One observation that's worth a note (non-blocking):
This PR newly invokes page-component getInitialProps in the prod resolver — before this change, resolvePagesPageData() never called page-level gIP at all, so a page exporting only getInitialProps (no gSP/gSSP) was previously SSR-rendered in prod without its initial props. The new behavior is the Next.js-correct one (gIP runs server-side on initial render), and it's well-tested. But the PR description frames the change as primarily about notFound/_error; this broader prod gIP enablement is a meaningful behavioral change that existing apps relying on client-only gIP fallback could notice. Worth calling out explicitly in the changelog/PR summary so it isn't a surprise.
See the one inline note below on dev/prod gating symmetry.
| if ( | ||
| typeof pageModule.getServerSideProps !== "function" && | ||
| typeof pageModule.getStaticProps !== "function" | ||
| ) { |
There was a problem hiding this comment.
Minor symmetry note (non-blocking): the prod resolver now gates the gIP block on hasPagesGetInitialProps(...) (pages-page-data.ts:687), but this dev block only checks the absence of gSP/gSSP and relies on loadPagesGetInitialProps returning null early. Functionally fine in dev (no createGsspReqRes allocation here), but adding the same hasPagesGetInitialProps(PageComponent) guard would keep dev/prod reading identically and make the intent obvious. Up to you.
|
Review posted successfully to PR #1737. SummaryRe-reviewed PR #1737 ( Verdict: Clean, correct, well-scoped change. No blocking issues. All three prior-review follow-ups were resolved appropriately. Verification performed locally (all pass):
What I confirmed:
New observation raised (non-blocking):
|
2 participants
Overview
notFoundand the HTML request must render the configured 404/error page.resolvePagesPageData()now returns an HTMLnotFoundsignal instead of constructing the built-in 404 response directly.pages/404orpages/_error, while data requests still return the JSON-shaped 404 envelope.pages-page-data.ts,pages-server-entry.ts,pages-get-initial-props.ts,dev-server.ts,tests/pages-router.test.ts._error.getInitialProps(ctx)receives the original request URL andasPathforgetStaticPropsorgetServerSidePropsnotFoundresults.Why
A Pages Router
notFoundresult is not the final HTML response. In Next.js,getStaticPropsmarks render metadata as not found, the Pages route handler delegates torender404(), and the server then resolvespages/404before falling back topages/_error. That fallback render still builds the normalNextPageContext, includingctx.req,ctx.res, andctx.asPath.notFoundnotFoundreturns{ kind: "notFound" }; only/_next/datarequests keep returning{}with status 404.pages/404andpages/_errorshould be resolved by the Pages entry with the original visible URL.statusCode: 404and the originalasPath.getInitialPropscontextgetInitialPropsbelongs at the page render boundary and should observe sent-response short-circuits.getInitialPropsin prod and dev without widening production code throughanyor broad assertions.What changed
getStaticPropsreturns{ notFound: true }for an HTML requestpages/_error.pages/404orpages/_errorand preserves 404 status._error.getInitialProps(ctx)on that rerenderctx.req?.urlandctx.asPathnever reached the page.req.urlandasPathfrom the original request, matching the upstream fixture./_next/data/...jsonnot foundMaintainer review path
packages/vinext/src/server/pages-page-data.ts- the resolver now emits a typednotFoundsignal for HTML while preserving JSON data-request behavior.packages/vinext/src/entries/pages-server-entry.ts- the generated Pages runtime reroutes that signal to explicit/404or/_error, avoiding catch-all matches and recursion.packages/vinext/src/server/pages-get-initial-props.ts- shared typed loader for Pages componentgetInitialProps, including the sent-response exception from Next.jsloadGetInitialProps.packages/vinext/src/server/dev-server.ts- dev parity for page and error-pagegetInitialPropscontext.tests/pages-router.test.tsandtests/pages-page-data.test.ts- upstream fixture port plus lower-boundary contract coverage.Validation
vp checkvp test run tests/pages-page-data.test.tsvp test run tests/pages-router.test.tsvp test run tests/entry-templates.test.tsknipvp env exec --node 24 ./scripts/run-nextjs-deploy-suite.sh /Users/nathan/Projects/vinext/.refs/nextjs-v16.2.6 --retries 0 -c 1 --debug test/e2e/error-handler-not-found-req-url/error-handler-not-found-req-url.test.tsRisk / compatibility
notFoundhandling and Pages componentgetInitialPropsinvocation when no gSP/gSSP owns props.notFoundfallback topages/404andpages/_error.notFoundwill now see their configured error page, which is the Next.js-compatible behavior.References
getStaticPropsnotFoundmetadatanotFoundbecoming render metadata instead of an immediate built-in response.metadata.isNotFoundisNotFoundtorender404()render404and error fallback/404then/_errorfallback sequence.loadGetInitialPropsctx.req,ctx.res, andctx.asPathare part of the error render context.loadGetInitialPropsgetStaticProps.notFoundnotFoundshould render a 404 page.