fix: await top-level await module imports#1381
Conversation
Next.js's `next/document` default export is `class Document extends React.Component`. vinext's shim exported a function, so user `_document.tsx` files that use the class-based form (the common Next.js idiom: `class MyDocument extends Document`) ended up extending a function. React refuses to call class constructors without `new`, which caused SSR to throw "Class constructor MyDocument cannot be invoked without 'new'" — surfacing in e2e as 500 / empty responses. This is the actual root cause of the "async modules render empty pages" report (issue #1361): the Next.js `test/e2e/async-modules` fixture's `pages/_document.jsx` uses `class extends Document`, and once it crashes the whole page renders empty. Top-level await itself propagates correctly through Rolldown's static-import graph already. Adds a class-based Document shim that mirrors Next.js's signature (static `getInitialProps`, instance `render()`), plus production-build e2e coverage for pages, `_app.tsx`, `getStaticProps`, `getServerSideProps`, API routes, and class-based `_document.tsx`, all using top-level `await`. Closes #1361 Ports from Next.js: test/e2e/async-modules/index.test.ts https://github.com/vercel/next.js/blob/canary/test/e2e/async-modules/index.test.ts
commit: |
|
|
/bigbonk review |
There was a problem hiding this comment.
Review: fix: await top-level await module imports
Summary
The diagnosis is correct and the fix is the right one: converting the next/document default export from a function to a class is necessary for class MyDocument extends Document to work. The test coverage is solid — production build tests covering _app, page, gssp, gsp, API route, and class-based _document with TLA, plus dev smoke tests for both routers. Good work overall.
A few things worth addressing:
1. getInitialProps signature diverges from Next.js
packages/vinext/src/shims/document.tsx:74
The Next.js default getInitialProps is:
static getInitialProps(ctx: DocumentContext): Promise<DocumentInitialProps> {
return ctx.defaultGetInitialProps(ctx)
}The vinext shim has:
static async getInitialProps(_ctx: unknown): Promise<Record<string, unknown>> {
return {};
}Two issues:
- Parameter type is
unknown— should be typed more precisely (at minimum accept{ defaultGetInitialProps?: ... }or aDocumentContext-like shape). User code that callsDocument.getInitialProps(ctx)from a subclassgetInitialPropswill work at runtime, but TypeScript consumers won't get any type information. - Return type is
Record<string, unknown>instead ofDocumentInitialProps(which hashtml: string; head?: JSX.Element[]; styles?: ...). This matters for any user code that destructures the result ofawait Document.getInitialProps(ctx).
The PR description acknowledges getInitialProps isn't wired up yet, so this is pre-existing. But since you're already touching this class, it would be a good time to at least match the parameter and return types, even if the implementation remains a stub. That way the next person wiring it up doesn't have to change the signature.
Not blocking — document it as a follow-up if you'd rather not do it here.
2. next-shims.d.ts doesn't declare the default export
packages/vinext/src/shims/next-shims.d.ts:57-63
The next/document module declaration only exports Html, Head, Main, NextScript — there's no default export declared. Now that the default export is a class with meaningful static methods and generics, TypeScript consumers using import Document from "next/document" in strict mode will get any or an error depending on their tsconfig. Consider adding:
declare module "next/document" {
import { Component, ComponentType, ReactNode } from "react";
export const Html: ComponentType<{ lang?: string; children?: ReactNode; [key: string]: unknown }>;
export const Head: ComponentType<{ children?: ReactNode }>;
export const Main: ComponentType;
export const NextScript: ComponentType;
export default class Document<P = unknown> extends Component<P & { children?: ReactNode }> {
static getInitialProps(ctx: unknown): Promise<Record<string, unknown>>;
render(): ReactNode;
}
}3. document.test.ts should get a class-extends test
tests/document.test.ts
The existing unit test file for the Document shim wasn't updated. Since the entire motivation for this PR is that class MyDocument extends Document must work, a direct unit test for that contract would be valuable here — something like:
it("can be extended by a user class that React can construct", () => {
class MyDocument extends Document {
render() {
return React.createElement(Html, { lang: "ja" },
React.createElement(Head),
React.createElement("body", null,
React.createElement(Main),
React.createElement(NextScript)));
}
}
const html = render(React.createElement(MyDocument));
expect(html).toMatch(/<html[^>]*lang="ja"/);
expect(html).toContain("__NEXT_MAIN__");
});This is the contract that broke and caused the issue — it deserves a targeted unit test, not just integration coverage.
4. The pages-basic fixture _document.tsx is function-based — intentional?
tests/fixtures/pages-basic/pages/_document.tsx
The existing fixture uses export default function Document() (not extending the class). This is fine for the existing tests, but it means the dev-mode TLA test at pages-router.test.ts:269 doesn't exercise the class-extends path that was actually broken. The integration test catches TLA rendering but not the specific extends Document failure that motivated this PR.
The production test block does exercise the class-based path (via the inline _document.tsx fixture), so coverage exists — just noting that the dev integration test is testing a slightly different code path.
5. App Router fixture comment references Pages Router source
tests/fixtures/app-basic/app/async-modules-test/page.tsx:3-4
// Ported from Next.js: test/e2e/async-modules/pages/index.jsx
This is an App Router page, but the comment references a Pages Router fixture (pages/index.jsx). The Next.js async-modules test suite is Pages-Router-only (it doesn't have an App Router equivalent). This isn't wrong per se (the TLA pattern is the same), but it's slightly misleading. Consider updating to:
// Based on Next.js: test/e2e/async-modules/pages/index.jsx (adapted for App Router)
6. Generic default: unknown vs {}
packages/vinext/src/shims/document.tsx:66
export default class Document<P = unknown> extends React.Component<P & { children?: ReactNode }>Next.js uses <P = {}>. Using unknown means P & { children?: ReactNode } collapses to { children?: ReactNode } in the default case (since unknown & T = T), which is fine at runtime. But {} is the conventional default for React component generics and matches what user code expects when they write class MyDoc extends Document without a type argument. Minor — but {} would be more consistent with Next.js.
Things that look good
- The core fix is correct: the function → class conversion fixes the
extendschain for React class component instantiation. - The JSDoc on the class explains the "why" clearly — future contributors will understand why this must be a class.
- The production test block is thorough: it builds a complete Pages Router app with TLA in
_app,_document, page, gssp, gsp, and API route, then tests all paths. This is exactly the right level of coverage for a production build regression. - The PR description's root cause analysis is excellent — it traces from the symptom (empty content) through the actual failure (class constructor invocation) to the fix.
- The link to the Next.js source reference is helpful.
|
Review posted on PR #1381. Here's a summary of the findings: The fix is correct — converting the Key feedback points:
|
…ntract Addresses Bonk review feedback on PR #1381: - Export DocumentContext / DocumentInitialProps types matching Next.js, so subclasses that delegate via `await Document.getInitialProps(ctx)` receive typed objects. The runtime path is still a stub (the Pages Router renderPage/defaultGetInitialProps chain is not wired up), but the signatures pin the contract. - Declare the default Document class export in next-shims.d.ts so strict-mode consumers don't see `any` for `import Document from "next/document"`. - Add a class-extends regression test in tests/document.test.ts — this is the contract that broke in issue #1361 (the original Next.js `pages/_document.jsx` uses `class extends Document`). - Switch the class generic default from `unknown` to `{}` to match Next.js's `class Document<P = {}>`. - Clarify App Router fixture comment: the upstream Next.js async-modules suite is Pages-Router-only, so this is adapted, not ported verbatim.
|
/bigbonk review |
There was a problem hiding this comment.
Review: fix: await top-level await module imports
LGTM. CI is fully green (all 37 checks pass), the root cause analysis is accurate, and the fix is correct.
The fix
Converting the next/document default export from a function to a class is the right solution. The failure mode is well-understood: class MyDocument extends <function> produces a class whose constructor chain breaks when React tries to instantiate it via new, because the parent "constructor" is a plain function that React calls without new. The class form restores the extends React.Component chain that user code expects.
Dev/prod parity
Verified that all Document rendering paths use React.createElement(DocumentComponent), which handles both function and class components identically:
- Dev server:
streamPageToResponseandrenderErrorPageinserver/dev-server.ts - Prod server: delegated through
pages-page-response.tsvia the generated entry
Neither path calls getInitialProps — the Document is always rendered directly as JSX with no props. The function-to-class change is transparent to all consumption sites. No parity issues.
Test coverage
Solid coverage across all layers:
- Unit test (
document.test.ts): Directclass MyDocument extends Documenttest that exercises the exact contract that broke, plusgetInitialPropsshape verification. This is the most important test in the PR — it pins the specific failure mode. - Dev integration (
pages-router.test.ts,app-router.test.ts): TLA fixture pages in bothpages-basicandapp-basicverify that dev-mode rendering works with top-level await. - Production integration (
pages-router.test.ts): Full temp-dir build with_app.tsx+ class-based_document.tsx+ page + gssp + gsp + API route, all with TLA. Covers the exact Next.jstest/e2e/async-modulesfixture shape.
The one coverage gap worth noting (non-blocking): the dev-mode Pages Router test at line 269 hits the pages-basic fixture, which uses a function-based _document.tsx that doesn't extend the class. So the dev integration test doesn't exercise the extends Document path. The production test block does cover it. This is fine — the unit test is the right place to pin the class contract, and the production test verifies the full E2E path.
Type declarations
The next-shims.d.ts update is correct — DocumentContext, DocumentInitialProps, and the default class Document export are all declared. TypeScript consumers using import Document from "next/document" will get proper type information.
Minor observations (non-blocking, for awareness)
-
getInitialPropsis never called by vinext's SSR pipeline. The stub returning{ html: "" }prevents runtime crashes for user code that delegates viaawait Document.getInitialProps(ctx), but the values won't reachthis.props. This is pre-existing and documented in the PR description and code comments. TherenderPage/defaultGetInitialPropspipeline is a separate piece of work. -
DocumentContextfields are all optional. This is the correct choice given that vinext doesn't plumb them through yet. It avoids forcing user code to assert their presence while maintaining type compatibility. -
The App Router fixture comment ("Based on Next.js: test/e2e/async-modules/pages/index.jsx") clarifies in the parenthetical that it's adapted for App Router, which is sufficient context.
Ship it.
|
Review posted on PR #1381 with approval. Summary of findings:
The |
…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
…SSR (#1633) * fix(pages-router): invoke Document.getInitialProps so docProps reach 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 * fix(pages-router): address bonk review on Document.getInitialProps helper 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
Summary
Issue #1361 reports that pages using top-level
await(async modules) render empty content in deployed Next.js compatibility tests (test/e2e/async-modules). After reproducing locally with the Next.js fixture, the actual root cause is not TLA propagation — it is thenext/documentshim default export being a function instead of a class.The original Next.js fixture's
pages/_document.jsxuses the standard class form:Next.js's
Documentdefault export isclass Document extends React.Component. vinext's shim exported a function.class MyDocument extends <function>is constructible at the JS level, but when React tries to render<MyDocument />it goes through the class-component path, hits theextendschain via the originalfunction Document(), and ends up calling the constructor withoutnew— throwingTypeError: Class constructor MyDocument cannot be invoked without 'new'. SSR 500s, the page body is empty, and every async-modules e2e expectation against#app-value,#page-value,#gssp-value,#gsp-value,#content-404etc. fails simultaneously. Top-levelawaititself propagates correctly through Rolldown's static-import graph already — Vinext's own pages,_app.tsx,getStaticProps,getServerSideProps, and API-route paths all render with TLA fine once_documentno longer crashes.This PR:
packages/vinext/src/shims/document.tsxdefault export to a class matching Next.js's signature (staticgetInitialProps, instancerender()).test/e2e/async-modulesfixture: index page +_app.tsx+gssp.tsx+gsp.tsx+api/hello.ts+ class-based_document.tsx, all using module-levelawait.tests/fixtures/{pages-basic,app-basic}/.../async-modules-test) so future regressions in TLA propagation surface in the targeted test files.Closes #1361
Files changed
packages/vinext/src/shims/document.tsx— class-based default exporttests/pages-router.test.ts— new "Pages Router top-level await (async modules) in production" describe block (5 tests) + dev integration testtests/app-router.test.ts— dev integration test for app-router TLAtests/fixtures/pages-basic/pages/async-modules-test.tsx— Pages Router TLA fixturetests/fixtures/app-basic/app/async-modules-test/page.tsx— App Router TLA fixtureTest plan
pnpm test tests/pages-router.test.ts— all 215 pass (5 new TLA tests)pnpm test tests/app-router.test.ts— all pass (1 new TLA test)pnpm test tests/shims.test.ts— all pass (Document class change doesn't break existing shim consumers)pnpm test tests/features.test.ts— 303 passpnpm run check— clean (format + lint + types)/bigbonk reviewNotes / caveats
_document.getInitialProps()during SSR is not wired up in vinext yet (separate gap, separate issue). The class-based shim restores the constructor contract so user_document.tsxfiles render at all, but values produced bygetInitialPropswon't reachthis.propsuntil that pipeline lands. Thecsr async page modulescase from the Next.js test (#doc-value: "doc value") depends on that follow-up. The TLA value itself is captured at module load.packages/next/src/pages/_document.tsx— default export isclass Document<P> extends React.Component<DocumentProps & P>with a staticgetInitialProps(ctx)that delegates toctx.defaultGetInitialProps.test/e2e/async-modules/index.test.ts.