fix(pages): apply Document renderPage enhancers - #2034
Conversation
commit: |
|
|
/bigbonk review for issues |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: fix(pages): apply Document renderPage enhancers
Reviewed all 9 changed files against main. The renderPage enhancer support, the Document.getInitialProps -> ctx.defaultGetInitialProps delegation, the single-invocation contract (runDocumentRenderPage vs loadUserDocumentInitialProps), and the dev/prod parity wiring all look correct and well-tested. The 12 new enhancer/error tests pass and meaningfully cover the happy paths plus enhancer/page/invalid-html/style-serialization error routing.
One regression worth fixing before merge, plus a couple of minor notes.
Potential regression: dev error page can now write a partial response, then swallow a mid-stream error
In renderErrorPage (packages/vinext/src/server/dev-server.ts), the custom-_document branch now delegates to streamPageToResponse(...) inside the per-candidate try. streamPageToResponse calls res.writeHead() + res.write(prefix) and then pipes the body stream to res.
On main, the entire error HTML was buffered into a string and written exactly once at the very end of the try (a single res.writeHead/res.end). So any render error happened before a single byte was written, and the catch { continue; } could safely fall through to the next candidate or the default fallback.
With this change, if streamPageToResponse throws after the shell has been flushed — i.e. the error _document takes the skipped (streaming) path and the error element throws from a deferred/Suspense boundary mid-pipe (reader.read() at the body-stream loop) — the exception propagates to:
} catch {
// This candidate doesn't exist, try next
continue;
}That catch silently swallows the streaming error and continues. Headers are already sent and a partial body is on the wire, so the next candidate (or the default fallback at the bottom, which calls res.writeHead(...) with no res.headersSent guard) throws ERR_HTTP_HEADERS_SENT. That re-throw escapes to the outer caller's fallback (res.end(...)), which also fails because the response is already partially written — net result is a corrupted/truncated error response and a confusing crash instead of a clean error page.
Note the rendered path is safe by construction (the body is rendered to a string in runDocumentRenderPage before any write), and getInitialProps/styles errors are likewise pre-write. The narrow-but-real window is the skipped+streaming path for an error _document whose body throws after the shell flush — which main handled safely via full buffering.
Suggested fix: after streamPageToResponse either guard the loop/default-fallback with if (res.headersSent || res.writableEnded) return;, and/or scope the swallowing catch so it only continues for the "candidate doesn't exist / module not found" case (which is what the comment claims it is for) and re-throws once a response write has begun. As written, the catch conflates "candidate not found" with "render failed after we started responding."
Minor
-
loadUserDocumentInitialPropssuppliesdefaultGetInitialProps: async () => ({ html: "" }). Now that baseDocument.getInitialPropsdelegates toctx.defaultGetInitialProps(ctx), any code path that reaches this compatibility helper with a user_documentthat overridesgetInitialPropsand callsawait Document.getInitialProps(ctx)would render an empty body. In practice both dev and prod main-render paths always passenhancePageElement, so the override always routes throughrunDocumentRenderPage(rendered) and this helper only returns non-null for... nothing reachable with an override (the identity check returnsnullfor the un-overridden base). It's effectively dead for the override case today, but it's a latent foot-gun if a future caller invokes this helper without wiringenhancePageElement. A short comment or an assertion would help. -
runDocumentRenderPagedestructuresdocPropsfromdocInitialProps ?? {}(line ~227) before thetypeof docInitialProps.html !== "string"validation throws (line ~230). Functionally fine since the throw happens beforedocPropsis used, but reordering the validation ahead of the destructure would read more clearly.
Everything else (typings tightening in next-shims.d.ts, err instanceof Error narrowing in pages-page-handler.ts, the query threading for DocumentContext, the prod resolvedDocProps reuse to avoid double-invocation) looks correct.
|
Review posted to PR #2034. SummaryI reviewed all 9 changed files in PR #2034 against The core feature work is solid and well-tested: renderPage enhancer support (object + legacy function forms), the I flagged one regression and two minor notes: Regression (worth fixing before merge): In Minor:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): apply Document renderPage enhancers
Re-reviewed the 9 changed files against main and ran the document/response suites (tests/document.test.ts, tests/pages-page-response.test.ts — 59 tests pass). The core feature work is correct and well-covered: the renderPage enhancer support (object + legacy function forms), the Document.getInitialProps → ctx.defaultGetInitialProps delegation, the single-invocation contract between runDocumentRenderPage and loadUserDocumentInitialProps, the prod resolvedDocProps reuse, the query threading into DocumentContext, and the typings tightening in next-shims.d.ts / document.tsx.
One confirmed regression remains, plus the two minor notes from the prior round.
Confirmed regression: dev error page can flush a partial response, then swallow a mid-stream throw
In renderErrorPage (dev-server.ts), the custom-_document branch now delegates to streamPageToResponse(...) inside the per-candidate try (line ~1952). streamPageToResponse calls res.writeHead() (line 349) + res.write(prefix) (line 352), then pipes the body stream in the reader.read() loop (lines 357–364).
On main, the error HTML was fully buffered to a string and written exactly once at the end (res.writeHead/res.end), so any render error happened before the first byte and catch { continue; } could safely fall through.
The narrow-but-real window: when the error _document does not override getInitialProps/renderPage (or the enhancer isn't routed), runDocumentRenderPage returns skipped, so bodyStream = await renderToReadableStream(element) (line 256) is the live React stream. If the error element throws from a deferred/Suspense boundary mid-pipe, the exception escapes streamPageToResponse after the shell has been flushed, and lands in:
} catch {
// This candidate doesn't exist, try next
continue;
}Headers + prefix are already on the wire, so:
- if another candidate remains (e.g.
500failed → tries_error), the nextstreamPageToResponsecall hitsres.writeHeadagain →ERR_HTTP_HEADERS_SENT; or - the loop ends and the default fallback at the bottom calls
res.writeHead(...)(lines ~2019 / ~2023) with nores.headersSent/res.writableEndedguard → same throw.
That second throw escapes to the outer caller's fallback (res.end(...) at line ~1837), which also operates on an already-partially-written response. Net result: corrupted/truncated error response plus a confusing secondary crash instead of a clean error page.
Note: the rendered path (and getInitialProps/styles/invalid-html errors) are safe by construction — the body is rendered to a string in runDocumentRenderPage before any write, which is why the new error-routing tests pass. They exercise the rendered path only; the skipped+streaming error-document body throw is untested. This is dev-only (prod buffers via renderPagesPageResponse), but it's a behavioral regression vs main.
Suggested fix: guard the loop iterations and the default fallback with if (res.headersSent || res.writableEnded) return; after any write may have begun, and/or scope the swallowing catch so it only continues for the "candidate doesn't exist / module not found" case (which is what the comment claims) and re-throws once a response write has started. As written, the catch conflates "candidate not found" with "render failed after we started responding." (The existing res.headersSent guard at line ~1902 is pre-stream only and doesn't cover this.)
Minor
-
loadUserDocumentInitialPropssuppliesdefaultGetInitialProps: async () => ({ html: "" })(line 76). Now that baseDocument.getInitialPropsdelegates toctx.defaultGetInitialProps(ctx), any future caller that reaches this compatibility helper with a user_documentthat overridesgetInitialPropsand callsawait Document.getInitialProps(ctx)would render an empty body. Today this is unreachable for the override case — every main render path wiresenhancePageElement, so overrides always route throughrunDocumentRenderPage(rendered), and the identity check returnsnullfor the un-overridden base. It's effectively dead for the override case, but a latent foot-gun; a short comment or assertion that this path must only be hit for non-renderPage props would help. -
runDocumentRenderPagedestructuresdocPropsfromdocInitialProps ?? {}(line 227) before thetypeof docInitialProps.html !== "string"validation throws (line 230). Functionally fine sincedocPropsisn't used before the throw, but moving the validation ahead of the destructure would read more clearly.
Everything else (the err instanceof Error narrowing in pages-page-handler.ts, the query/err threading, and the comment cleanup dropping the stale consumed status) looks correct.
|
Review posted to PR #2034. SummaryI re-reviewed all 9 changed files against Confirmed one regression (worth fixing before merge): In I also corrected the prior review's claim: there is a Two minor notes (carried from the prior round, both verified): the Everything else — the enhancer support, the delegation, single-invocation contract, prod |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): apply Document renderPage enhancers
Re-reviewed against main after commit d8b620c ("buffer custom dev error documents"). Ran tests/document.test.ts + tests/pages-page-response.test.ts (59 pass) and vp check on the changed source files (clean). The core feature work — renderPage enhancer support (object + legacy function forms), the Document.getInitialProps -> ctx.defaultGetInitialProps delegation, the single-invocation contract between runDocumentRenderPage and loadUserDocumentInitialProps, prod resolvedDocProps reuse, query/err threading, and the typings tightening — all look correct.
Prior-round regression: now mitigated (not a blocker)
The dev error-page streaming regression flagged in earlier rounds is addressed by d8b620c, but worth confirming the shape of the fix since it's partial:
- The main render path (
dev-server.ts:1771) now passesbufferBodyBeforeHeaders: true, so the body is fully buffered (await new Response(bodyStream).text()at line 334) beforeres.writeHead()at line 353. A mid-render throw there now happens pre-write. Good. - The error-page path (
renderErrorPage,dev-server.ts:1962) does not passbufferBodyBeforeHeaders, so it still streams. But the new guard at line 2013 —if (res.headersSent || res.writableEnded) return;inside the per-candidatecatch— prevents the previously-reportedERR_HTTP_HEADERS_SENTcascade: a mid-stream throw after the shell flush now returns cleanly instead ofcontinueing to the next candidate / default fallback (lines ~2030/2034) and re-callingres.writeHead.
Net: the crash is fixed. The error response can still be truncated in the narrow skipped+streaming-throw case (error _document that doesn't override getInitialProps/renderPage, with a body that throws from a deferred/Suspense boundary), since that path isn't buffered. That's dev-only and inherent to streaming, so it's acceptable — but if you want full parity with the old buffered behavior, passing bufferBodyBeforeHeaders: true on the renderErrorPage streamPageToResponse call too would close it entirely (error pages are small; buffering them is cheap). Not a merge blocker.
Minor (carried, both verified)
-
loadUserDocumentInitialPropshtml: ""stub is a latent foot-gun.pages-document-initial-props.ts:76suppliesdefaultGetInitialProps: async () => ({ html: "" }). Now that baseDocument.getInitialPropsdelegates toctx.defaultGetInitialProps(ctx), a user_documentthat overridesgetInitialPropsand callsawait Document.getInitialProps(ctx)through this helper renders an empty body.tests/document.test.ts:165now pins this toexpect(props!.html).toBe(""). It's unreachable for the override case today — every main render path wiresenhancePageElement, so overrides always route throughrunDocumentRenderPage(rendered), and the identity check returnsnullfor the un-overridden base — but a future caller that invokes this helper without wiringenhancePageElementwould silently get an empty body. A short comment or assertion that this path must only be reached for non-renderPage props would guard against that. -
runDocumentRenderPagedestructuresdocPropsbefore thehtmlvalidation.pages-document-initial-props.ts:227destructures{ html, head, styles, ...docProps } = docInitialProps ?? {}before thetypeof docInitialProps.html !== "string"throw at line 230. Functionally fine (the throw precedes any use ofdocProps), but moving the validation ahead of the destructure reads more clearly.
Everything else (the err instanceof Error narrowing in pages-page-handler.ts, the query ?? params fallback in pages-page-response.ts:524, and the stale-consumed comment cleanup) looks correct.
|
Review posted to PR #2034. SummaryRe-reviewed all 9 changed files against The prior-round regression is now mitigated (not a blocker). Commit
The error-page path itself still streams (it does not pass Two minor notes (both verified, carried from prior rounds):
Everything else (the |
1 participant
Summary
ctx.renderPageenhancersDocument.getInitialProps(ctx)delegate toctx.defaultGetInitialProps(ctx)for canonical CSS-in-JS integrationsrenderPageis intentionally skippedNext.js parity
Fixes the
_document.getInitialProps().renderPage()enhancer parity failures and supports documented styled-components/emotion integration patterns.Validation
vp checkand vinext build passedCache Components, PPR, and resume behavior are out of scope.