feat: implement next/document with class-based Document and getInitialProps support#594
feat: implement next/document with class-based Document and getInitialProps support#594james-elicx wants to merge 4 commits intomainfrom
Conversation
…lProps support - Rewrite next/document shim to export DocumentContext, DocumentInitialProps types and a class-based Document with default getInitialProps - Add declare module 'next/document' ambient types in next-shims.d.ts - Wire getInitialProps into dev-server streamPageToResponse via buildDocumentContext helper - Add test fixture _document.tsx with custom getInitialProps that injects a theme prop - Add E2E test verifying the theme prop is rendered on the body element
commit: |
|
|
/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: feat: implement next/document with class-based Document and getInitialProps support
Good progress on implementing getInitialProps for the Document class. The shim types and class-based pattern are well-structured. However, there are a few issues that should be addressed before merging.
1. Dev/prod parity: getInitialProps not wired in production Pages Router
This is the main concern. The AGENTS.md is explicit about this:
When fixing a bug in any of these files, check whether the same bug exists in the others. Do not leave known bugs as "follow-ups" — fix them in the same PR.
The production Pages Router server entry (packages/vinext/src/entries/pages-server-entry.ts, line 1023-1024) renders the Document component without calling getInitialProps:
if (DocumentComponent) {
const docElement = React.createElement(DocumentComponent);This means getInitialProps works in dev but silently does nothing in production builds. A custom document that injects props (like the test fixture's data-theme-prop) will work in dev but break in prod. This is exactly the kind of dev/prod divergence the codebase guidelines warn about.
The same issue exists in renderErrorPage within dev-server.ts itself (line 1180-1181) — error pages skip getInitialProps even in dev.
2. Silent error swallowing in getInitialProps
try {
const ctx = buildDocumentContext(req, url);
docProps = await DocClass.getInitialProps(ctx);
} catch {
// If getInitialProps fails, fall back to rendering with no props
}Silently swallowing errors from user code is dangerous — it makes bugs invisible. Next.js does not silently catch getInitialProps errors; it surfaces them as 500 errors. At minimum, log the error with console.error. In dev mode, this should probably propagate to show the error overlay, matching Next.js behavior.
3. defaultGetInitialProps signature doesn't match Next.js
In the DocumentContext type:
defaultGetInitialProps(
ctx: DocumentContext,
options?: { nonce?: string },
): Promise<DocumentInitialProps>;But buildDocumentContext creates a defaultGetInitialProps that ignores its arguments:
const defaultGetInitialProps = async (): Promise<DocumentInitialProps> => ({
html: "",
});This works because JS allows calling a function with more args than it declares, but it means the ctx parameter in the type is misleading. In Next.js, defaultGetInitialProps actually calls ctx.renderPage() internally. The implementation should at least call renderPage() for correctness:
const defaultGetInitialProps = async (ctx: DocumentContext): Promise<DocumentInitialProps> => {
return ctx.renderPage();
};4. Test coverage gaps
The E2E test only covers the happy path (class-based document with getInitialProps that succeeds). Missing test cases:
- Function-based document without
getInitialProps: This is a regression risk — does the oldexport default function Document() {}pattern still work after this change? The Document class now has agetInitialPropson the base class, and the check istypeof DocClass.getInitialProps === "function". For a plain function export, this should beundefined, but it's worth verifying. - Error page rendering: Error pages (
renderErrorPage) don't callgetInitialProps— is that intentional? Next.js calls it for error pages too.
Minor notes
- The
buildDocumentContextdoes not passres(the ServerResponse) to the context. Some custom documents usectx.resto set headers. Consider including it. - The duplicate type definitions between
document.tsxandnext-shims.d.tsare a maintenance risk — if one is updated and the other isn't, they'll drift. Consider having the.d.tsreference the actual implementation types or add a comment cross-referencing them.
|
I've posted a review on PR #594 requesting changes. Here's a summary of the key findings: Critical issueDev/prod parity gap — Other issues
|
- Wire getInitialProps into the production Pages Router entry (pages-server-entry.ts), fixing a dev/prod parity gap where custom document props were silently dropped in production - Wire getInitialProps into renderErrorPage in dev-server.ts so error pages (404, 500) also call the custom document's getInitialProps - Propagate getInitialProps errors instead of silently swallowing them; log and rethrow so bugs are visible - Fix defaultGetInitialProps to call ctx.renderPage() instead of returning a hardcoded empty string, matching Next.js semantics - Pass res (ServerResponse) to buildDocumentContext so custom documents can access ctx.res to set headers - Add E2E tests: error page uses custom document, basic document structure present, getInitialProps pathname context
|
/bigbonk review |
There was a problem hiding this comment.
Review
The overall design is solid — class-based Document with proper getInitialProps wiring across dev, production, and error pages. The previous review's main concern (dev/prod parity) has been addressed: getInitialProps is now called in pages-server-entry.ts, streamPageToResponse, and renderErrorPage. Good.
There's one correctness bug that should be fixed, and a few things worth addressing.
1. Silent error swallowing in streamPageToResponse (dev)
In streamPageToResponse (dev-server.ts:173-176), getInitialProps is called without any error handling:
if (typeof DocClass.getInitialProps === "function") {
const ctx = buildDocumentContext(req, url, res);
docProps = await DocClass.getInitialProps(ctx);
}If getInitialProps throws, the error propagates up and kills the request with an opaque stack trace. In contrast, the error page path (dev-server.ts:1186-1192) and the production path (pages-server-entry.ts:1039-1042) both have try/catch with console.error + re-throw, which gives a clear [vinext] prefixed message before the crash.
This should have the same try/catch + console.error + re-throw pattern for consistency. It's a dev-only path so the error overlay will pick it up either way, but the log message helps with debugging headless/CI scenarios.
2. Production 404 doesn't use custom _document
The production server returns a hardcoded HTML string for 404s:
// pages-server-entry.ts:736-737
return new Response("<!DOCTYPE html><html><body><h1>404 - Page not found</h1></body></html>",
{ status: 404, headers: { "Content-Type": "text/html" } });This means the E2E test for 404 + data-theme-prop only passes in dev (where renderErrorPage handles it). In production, a 404 won't use the custom _document at all. The test runs against the preview server (localhost:4173), so it depends on which server is actually running. Worth adding a comment in the test or documenting this as a known gap — production 404 rendering with custom _document is a separate piece of work, but the test shouldn't give a false impression that it's covered end-to-end.
3. renderPage returns empty html — document this intentional deviation
renderPage in both dev and prod always returns { html: "" }:
const renderPage = async () => ({ html: "" });In Next.js, renderPage() actually renders the app to HTML and returns it in the html field. Some custom documents rely on renderPage to, for example, collect styled-components sheets or Emotion cache. The vinext architecture handles this differently (the body is streamed separately), so renderPage is intentionally a no-op. This is fine, but:
- The
DocumentInitialProps.htmlfield being always""should be documented as an intentional deviation, not a TODO. Someone extendingDocumentwho callsctx.renderPage()and expects populatedhtmlwill get an empty string silently. - Consider adding a brief comment in
buildDocumentContextand the inline prod context explaining this.
4. Duplicated DocumentContext/DocumentInitialProps types
The same type definitions are written three times:
shims/document.tsx— runtime typesshims/next-shims.d.ts— ambient module declaration- Inline casts in
dev-server.tsandpages-server-entry.ts
The .d.ts and the .tsx will drift. Consider having next-shims.d.ts use export { DocumentContext, DocumentInitialProps } from "../shims/document.js" or at minimum adding a cross-reference comment like // Keep in sync with shims/document.tsx.
5. Minor: production context missing req and res
The dev context passes req and res (dev-server.ts:117-118), but the production context (pages-server-entry.ts:1031-1037) omits them. In the Cloudflare Workers runtime there's no Node IncomingMessage/ServerResponse, so this is arguably correct, but it should be documented. Some custom documents check ctx.req to detect server-side rendering vs client-side — they'll get undefined in production silently.
Tests
The E2E tests are well-structured. One suggestion: the about page test (line 36-44) doesn't actually verify anything about the pathname being different — data-theme-prop is always "light" regardless of pathname. If the intent is to verify DocumentContext.pathname works, the fixture's getInitialProps should use ctx.pathname in the returned props (e.g., dataPath: ctx.pathname) and the test should assert on that value.
|
Review posted on PR #594. Here's a summary of what I flagged:
|
…getInitialProps - Add renderErrorPageResponse() helper in pages-server-entry.ts that routes 404/500 responses through AppComponent and DocumentComponent, calling getInitialProps so custom _document styling and metadata applies to error pages - Replace all plain-HTML 404 fallbacks (no-match, getStaticPaths fallback:false, getServerSideProps notFound, getStaticProps notFound) with renderErrorPageResponse - Add try/catch + console.error around getInitialProps in dev-server.ts for consistency with the prod error-page path - Add cross-reference comment to next-shims.d.ts next/document declaration - Expand next-shims.d.ts in fixture to include full DocumentContext/DocumentProps types and Document base class so _document.tsx can use getInitialProps correctly - Update _document.tsx fixture to propagate ctx.pathname as data-pathname on body - Update document.spec.ts (dev + prod) to assert data-pathname attribute value
…opagation - Add full AppContext/AppInitialProps/AppTree types to next/app shim - Call App.getInitialProps in dev-server, pages-server-entry at all render sites - Serialize appProps into __NEXT_DATA__.props for client hydration - Spread appInitialProps in client/entry.ts initial hydration - Fix router.ts navigateClient to spread appInitialProps on SPA transitions - Fix ISR regen path: hoist _regenAppProps out of if(RegenApp) so it can be included in the freshNextData script - Add pages-router-prod document.spec.ts E2E tests - Update document test to check #app-wrapper (where _app renders data-app-prop)
1 participant
Summary
next/documentshim to export proper TypeScript types (DocumentContext,DocumentInitialProps) and a class-basedDocumentbase class with a defaultgetInitialPropsstatic methoddeclare module 'next/document'types tonext-shims.d.tsso custom_document.tsxfiles type-check correctlygetInitialPropsinto the Pages Router dev server via a newbuildDocumentContexthelper — custom documents can now augment document props (e.g. injecting a theme)_document.tsxto use a class-based document withgetInitialPropsthat adds athemeprop rendered asdata-theme-propon<body>tests/e2e/pages-router/document.spec.ts) that verifies the custom document prop is present in the rendered HTMLTesting
PLAYWRIGHT_PROJECT=pages-router pnpm test:e2e tests/e2e/pages-router/document.spec.ts— 1 passedPLAYWRIGHT_PROJECT=pages-router pnpm test:e2e— 85/85 passed, no regressions