fix(pages-router): invoke Document.getInitialProps so docProps reach SSR#1633
Conversation
|
/bigbonk review |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Good extraction — single helper, three call sites, correct fast-path short-circuit for unoverridden shims. Two issues: one incorrect behavioral claim that led to a wrong error-handling policy, and one per-request dynamic import on the hot path that can be avoided.
1. Error handling diverges from Next.js — comment is inaccurate
pages-document-initial-props.ts:72-76 says:
Next.js logs and continues without doc props when the user's getInitialProps throws.
This is incorrect. Next.js's loadGetInitialProps (in packages/next/src/shared/lib/utils.ts) has no try/catch around App.getInitialProps(ctx). If Document.getInitialProps throws, the error propagates uncaught through loadDocumentInitialProps → renderDocument → renderToHTMLImpl and surfaces as a 500. Next.js also explicitly throws if the result is falsy or missing .html:
if (!docProps || typeof docProps.html !== 'string') {
throw new Error(`"${getDisplayName(Document)}.getInitialProps()" should resolve to an object with a "html" prop...`)
}The current vinext implementation silently swallows the error and renders with no doc props, which means a user whose getInitialProps has a bug sees a subtly broken page instead of an error. This is harder to debug than a 500.
Suggested fix: Let the error propagate (remove the outer try/catch). If there's a deliberate reason to diverge from Next.js here (e.g., defense-in-depth for Workers where a 500 is more expensive), document it as a conscious divergence, not as matching Next.js. The test "swallows errors from the user getInitialProps so renders never crash" should be updated accordingly — either to expect a thrown error, or to document the divergence.
2. await import("vinext/shims/document") runs on every request
loadUserDocumentInitialProps dynamically imports the base shim on every call to compare getInitialProps identity. In dev this is fine (Vite caches modules), but in a production Cloudflare Worker this is a top-level await import() on every page render. The module is always the same — its identity never changes at runtime.
Suggested fix: Cache the resolved base getInitialProps in a module-scoped let so the import runs at most once:
let _baseGetInitialProps: unknown;
let _baseResolved = false;
// inside the function:
if (!_baseResolved) {
try {
const docMod = await import("vinext/shims/document");
_baseGetInitialProps = docMod.default?.getInitialProps ?? null;
} catch { /* shim resolution failed */ }
_baseResolved = true;
}
if (_baseGetInitialProps && getInitialProps === _baseGetInitialProps) return null;This keeps the lazy-init behavior (doesn't import until first call) while avoiding repeated dynamic imports on the hot path.
Everything else looks solid: the helper is well-documented, the three call sites are consistent, the "returns null for unoverridden shim" fast path is correct, the test coverage for the happy path and no-override path is good, and the 404 TLA test is a nice addition.
| // Surface but don't fail the render: Next.js logs and continues without | ||
| // doc props when the user's getInitialProps throws. | ||
| console.error("[vinext] Document.getInitialProps threw:", err); | ||
| return null; |
There was a problem hiding this comment.
The comment on this catch block says "matches Next.js's tolerant behaviour around document props" — but Next.js does not have tolerant behaviour here. loadGetInitialProps in packages/next/src/shared/lib/utils.ts has no try/catch; a throw from Document.getInitialProps propagates as a 500.
Suggest either: (a) let the error propagate (remove this catch) to match Next.js, or (b) keep the catch but fix the comment to say this is a deliberate vinext divergence and explain why.
| const docMod = (await import("vinext/shims/document")) as { | ||
| default?: { getInitialProps?: unknown }; | ||
| }; | ||
| baseGetInitialProps = docMod.default?.getInitialProps ?? null; |
There was a problem hiding this comment.
This await import("vinext/shims/document") runs on every request. Since the base shim's getInitialProps identity never changes at runtime, cache it in a module-scoped variable so the import executes at most once:
let _baseGetInitialProps: unknown;
let _baseResolved = false;
// then inside the function:
if (!_baseResolved) {
try {
const docMod = await import("vinext/shims/document");
_baseGetInitialProps = docMod.default?.getInitialProps ?? null;
} catch {}
_baseResolved = true;
}
|
Review posted on PR #1633 with request-changes status and two inline comments:
The rest of the PR — the helper extraction, three consistent call sites, fast-path short-circuit, test coverage, and the 404 TLA addition — looks solid. |
…lper Two changes from review feedback on PR #1633: 1. Cache the base getInitialProps reference at module level. The previous helper did `await import("vinext/shims/document")` on every request to compare identity — unnecessary per-request work and an extra await on the fast path where the user has no override. Switched to a static import + module-scoped constant so the identity check is a synchronous reference compare. No-override callers now resolve with one less await. 2. Let user `getInitialProps` errors propagate. Next.js's `loadGetInitialProps` does not catch — a throw surfaces as a 500 to the caller. The previous version logged-and-swallowed the error and returned null, which silently erased docProps on every failing render and made user bugs harder to find. vinext now matches Next.js's contract. Updated the corresponding unit test to assert the throw propagates rather than that the helper resolves to null. Refs #1361, #1633
|
/bigbonk review |
There was a problem hiding this comment.
Both prior review issues are resolved:
-
Error propagation — the try/catch is gone. User
getInitialPropserrors now propagate to the caller, matching Next.js'sloadGetInitialPropswhich has no catch. The test correctly assertsrejects.toThrow("boom")instead of the previous swallow-and-return-null behavior. -
Static import over per-request dynamic import —
import BaseDocument from "vinext/shims/document"+ a module-scopedBASE_GET_INITIAL_PROPSconstant replaces the per-requestawait import(...). This is cleaner than the lazy-init pattern I would have suggested — the identity is established once at module evaluation, no per-requestawaiton the fast path, and no mutable module-scoped state to reason about.
The helper is well-documented (including the DocumentContext limitation as an explicit follow-up), the three call sites are consistent, and the test coverage hits the three key paths: override, no-override, and error propagation. The 404 TLA addition is a nice bonus ported from the Next.js async-modules fixture.
LGTM.
|
Approval review posted on PR #1633. Both prior review issues are resolved — error propagation now matches Next.js's |
…SSR (#1361) `pages/_document.tsx` files that override `static async getInitialProps` to inject extra props were rendered with an empty `this.props`, because both the dev-server and the production pages response builder created the Document element with `React.createElement(DocumentComponent)` and never called the hook. The Next.js async-modules e2e fixture (`test/e2e/async-modules/pages/_document.jsx`) uses exactly that pattern to surface a `docValue` from `await Document.getInitialProps(ctx)` plus a top-level `await` constant. PR #1381 (issue #1361) fixed the class construction crash, but the deploy-suite continued to fail "csr async page modules" because `<div id="doc-value">` rendered empty. This change adds a small `loadUserDocumentInitialProps` helper used by both the Pages Router dev-server (`streamPageToResponse` shell and the custom-error-page shell) and the production response builder (`buildPagesShellHtml`). It invokes the override (if any) and spreads the resolved props on the Document element, mirroring Next.js's `render.tsx` `<Document {...htmlProps} {...docProps} />` call. The helper short-circuits when the user did not override the base shim, so the existing fast path keeps the same number of awaits. Reference: vercel/next.js packages/next/src/server/render.tsx (`loadDocumentInitialProps`, `documentElement`). Tests: - Adds a new pages-router production e2e case that builds a `_document.tsx` with `static async getInitialProps` + top-level `await`, plus a custom `404.tsx` that uses top-level `await`, and asserts the resolved values reach the rendered HTML. - Unit-tests `loadUserDocumentInitialProps` directly for the override, no-override, and throwing-override cases. Refs #1361
…lper Two changes from review feedback on PR #1633: 1. Cache the base getInitialProps reference at module level. The previous helper did `await import("vinext/shims/document")` on every request to compare identity — unnecessary per-request work and an extra await on the fast path where the user has no override. Switched to a static import + module-scoped constant so the identity check is a synchronous reference compare. No-override callers now resolve with one less await. 2. Let user `getInitialProps` errors propagate. Next.js's `loadGetInitialProps` does not catch — a throw surfaces as a 500 to the caller. The previous version logged-and-swallowed the error and returned null, which silently erased docProps on every failing render and made user bugs harder to find. vinext now matches Next.js's contract. Updated the corresponding unit test to assert the throw propagates rather than that the helper resolves to null. Refs #1361, #1633
james-elicx
force-pushed
the
fix/issue-1361-async-modules
branch
2 times, most recently
from
8e9d68f to
97d26a4
Compare
|
Rebased on main; conflict in |
1 participant
Summary
static async getInitialProps()on userpages/_document.tsxinto the Pages Router SSR pipeline sothis.propsincludes the resolved values at render time. Both the dev-server and the production response builder now invoke the hook and spread the result on the rendered Document element, matching Next.js's<Document {...htmlProps} {...docProps} />inpackages/next/src/server/render.tsx.loadUserDocumentInitialPropsso the dev page render, dev custom-error render, and productionbuildPagesShellHtmlall go through the same code path. The helper short-circuits when the user did not override the base shim, so the fast path keeps its existing number of awaits.Background
The Next.js
test/e2e/async-modulesfixture's_document.jsxuses the exact pattern:PR #1381 fixed the class-construction crash (the shim's default export now extends as a class). After that, the deploy-suite still reported "csr async page modules" failing because
<div id="doc-value">came through empty: the render call wasReact.createElement(DocumentComponent)with no props, sogetInitialPropswas never invoked.This PR closes that gap. Only
docPropsare forwarded; the fullDocumentContext(renderPage,defaultGetInitialProps,pathname,req,res, etc.) is still not plumbed through and is called out in the helper's docstring as a follow-up. The common upstream pattern —await Document.getInitialProps(ctx)+ spread — works because the base shim ignoresctx.Test plan
pnpm test tests/document.test.ts— coversloadUserDocumentInitialPropsfor override, no-override, and throw paths.pnpm test tests/pages-router.test.ts— 253/253 pass, including:_document.tsxfixture that usesstatic async getInitialProps+ module-levelawait, plus a new404.tsxTLA case.pnpm test tests/pages-page-response.test.ts tests/pages-default-404.test.ts— unaffected suites stay green.pnpm run check— formatting, lint, and type checks pass.Fixes #1361