fix(pages): preserve document import order#2560
Conversation
commit: |
|
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
/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): preserve document import order
Overall this is a well-scoped fix. The _document > _app > page ordering is correct in both the dev SSR runner and the generated production entry, and it matches Next.js module-evaluation order. The error-propagation change (removing the fail-open try/catch around root-module loads) is intentional, tested, and correctly degrades on the 404 path via the outer connect error handler (next(e) at packages/vinext/src/index.ts:5284) → 500. Nice porting of the Next.js app-document-import-order cases.
I found one behavioral divergence worth addressing before merge.
1. Data requests now eagerly import _document (parity regression)
In the dev SSR handler, loadPagesRootComponents() is called unconditionally on the main render path:
packages/vinext/src/server/dev-server.ts:813
const { DocumentComponent, AppComponent } = await loadPagesRootComponents(
runner,
pagesDir,
matcher,
);This runs for every page request, including isDataReq (/_next/data/<buildId>/<page>.json) requests, which reach this line and only short-circuit later (the isDataReq early returns are at lines 1508/1602-ish, all after 813).
Previously, _document was loaded lazily inside the HTML-render branch (old dev-server.ts line ~1863), which sits after all the isDataReq early returns. So in the old code a data request never evaluated _document; now it always does.
This diverges from Next.js, where _document is server-HTML-render-only and is never evaluated for data requests. Consequences:
- A
_documentwith import side effects now runs on data requests. - A throwing
_documentnow 500s data requests that previously succeeded (the_next/dataJSON path).
_document isn't used for the data-request response at all (it's only consumed in the HTML streamPageToResponse branch), so this is pure wasted work plus a side-effect/error-surface divergence.
Suggested fix: keep _app eager (it participates in readiness / getInitialProps and the __NEXT_DATA__ payload, matching the old ordering rationale), but defer _document loading to the HTML-render branch, or gate the _document portion on !isDataReq. That preserves the _document > _app > page ordering for HTML renders while restoring the "data requests don't touch _document" behavior. Worth a ported test asserting a throwing _document does not break a /_next/data/*.json request (Next.js oracle: data requests never evaluate _document).
Minor
loadPagesRootComponentsreturnsappAssetPathbut the two callers on the main render path (:813) destructure only{ DocumentComponent, AppComponent }; onlyrenderErrorPageusesappAssetPath. Fine as-is, just noting the asymmetry.
Everything else (ordering assertions in tests/entry-templates.test.ts, propagation tests in tests/pages-router.test.ts, prod parity via the generated entry — prod-server.ts doesn't load _document/_app itself) looks correct. Verified locally: the new order + propagation tests pass.
|
Review posted to PR #2560. SummaryThe PR correctly fixes I found one behavioral divergence worth addressing:
I also verified prod parity (prod-server delegates to the correctly-ordered generated entry) and ran the new ordering + propagation tests locally — all pass. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): preserve document import order
The head commit fec1a9b addresses the parity concern raised in my prior review — data requests no longer eagerly evaluate _document. This is a clean, well-tested fix. Verified locally: tests/entry-templates.test.ts (42 passed) and tests/pages-router.test.ts (357 passed), plus vp check on both modified source files.
What's correct
-
Ordering fix (both runtimes). The generated production entry now emits
docImportCode → appImportCode → pageImports(pages-server-entry.ts:293-295), and the dev runner loads_document→_app→ page vialoadPagesRootComponents+ the subsequentimportModule(runner, route.filePath)(dev-server.ts:815-826). Since these are static ESM imports (orconst X = nullfallbacks when the file is absent), textual order determines module-evaluation order. Matches Next.js. -
Data-request gate is safe.
loadPagesRootComponents(..., { includeDocument: !isDataReq })atdev-server.ts:817runs before the data-request short-circuits (main envelope return at:1631-1677, plus the redirect/notFound returns at:739/:940/:1098/:1550), all of which precede the HTML render at:1935whereDocumentComponentis consumed. SoDocumentComponent === nullon data paths is never read._appstays eager (participates in readiness/__NEXT_DATA__), matching the old rationale. The new test "does not evaluate _document for a dev data request" (throwing_document+/_next/data/*.json→ 200) locks this in. -
Error-propagation change is deliberate and covered. Removing the fail-open
try/catcharound root-module loads means a throwing_document/_appnow surfaces. On the 500 path this is caught by the fallback atdev-server.ts:2070-2076; on the 404/normal paths it propagates to the outernext(e)atindex.ts:5284→ 500. New tests assert this for both a normal route (throwing_document) and a cold-start 404 (throwing_app). This is dev-only (prod delegates to the generated entry) and matches Next.js fail-fast on broken root modules. -
findFileWithExtsrefactor is equivalent and slightly more robust — it returns the resolved absolute path and imports it directly, instead of the old existence-check-then-extensionless-import pattern. SamedottedExtensions/existsSynciteration order.
Minor (non-blocking)
-
Redundant
_applookup on the HTML render path.loadPagesRootComponentsalready computes and returnsappAssetPath, but the main-path caller destructures only{ DocumentComponent, AppComponent }(dev-server.ts:817) and then recomputes it at:1724(AppComponent ? findFileWithExts(...) : null). One extrafindFileWithExtsper HTML render — harmless, but you could thread the returnedappAssetPaththrough to drop the second lookup. -
renderErrorPagenow evaluates_document/_appeven when no error-page candidate exists.loadPagesRootComponentsruns unconditionally at:2109before the candidate loop, so a 404 that falls through tobuildDefaultPagesNotFoundResponse()(:2243) now evaluates root modules where it previously didn't. Behaviorally this only matters when a root module throws, which the new cold-start_apptest asserts should 500 — so it's intended and covered. Noting it only because it's a wider surface than the "matched error page" case.
Nothing blocking. LGTM.
|
Review posted to PR #2560. SummaryThe head commit Verified:
Correct behavior confirmed:
Two minor, non-blocking notes:
No blocking issues found. |
…ent-import-order-28938793088-v2
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): preserve document import order
Reviewed at head 3460f36. This is a clean, well-scoped fix. The _document > _app > page ordering is now correct in both the dev SSR runner and the generated production entry, and it matches the Next.js oracle. Verified locally: tests/entry-templates.test.ts (42 passed) + tests/pages-router.test.ts (357 passed), and vp check on both modified source files is clean.
What's correct
-
Ordering matches Next.js. Confirmed against
test/e2e/app-document-import-order/app-document-import-order.test.ts, which asserts side effects fire in['_document', '_app', 'page']order. The generated entry emitsdocImportCode → appImportCode → pageImports(pages-server-entry.ts:293-295) and the dev runner loads_document→_app(loadPagesRootComponents) then the page (importModule(runner, route.filePath)atdev-server.ts:826). Since these are static ESM imports (orconst X = nullfallbacks), textual order = module-evaluation order. -
Data-request
_documentgate is correct.loadPagesRootComponents(..., { includeDocument: !isDataReq })(dev-server.ts:817) runs before everyisDataReqshort-circuit, and the_next/dataJSON 404 exits at:753before ever reachingrenderErrorPage, so a data request never evaluates_document.DocumentComponent === nullon data paths is never consumed. The new "does not evaluate _document for a dev data request" test (throwing_document+/_next/data/*.json→ 200) locks this in. This resolves the parity concern from the earlier review round. -
Fail-fast propagation is deliberate, parity-correct, and covered. Removing the fail-open
try/catcharound root-module loads means a throwing_document/_appnow surfaces instead of being swallowed. This matches Next.js (broken root modules fail the render, they don't silently degrade). Note the behavioral consequence inrenderErrorPage:loadPagesRootComponentsat:2109is now outside the per-candidatetry/catch, so a throwing_appturns a 404 into a 500 rather than continuing to the default 404 fallback. That is the intended fail-fast behavior and is asserted by the new "propagates a throwing _app import on a cold-start missing dev route" test. -
Dev/prod parity holds.
prod-server.tsandcloudflare/worker-entry.tsdon't load_document/_appthemselves — they delegate to the generated entry, which now carries the corrected order. No separate fix needed there. -
findFileWithExtsrefactor is equivalent. SamedottedExtensionsiteration order as the oldfindFileWithExtensionsexistence check, but returns the resolved absolute path and imports it directly. The_documentload also switches from an extensionlessrunner.import(docPath)to the resolved-with-extension path — more robust, and covered by the new dev import-order test using a real_document.tsx.
Minor (non-blocking)
-
Redundant
_applookup on the HTML render path.loadPagesRootComponentsalready resolves and returnsappAssetPath, but the main-path caller destructures only{ DocumentComponent, AppComponent }(dev-server.ts:817) and then recomputes it at:1724. One extrafindFileWithExtsper HTML render — harmless; you could thread the returnedappAssetPaththrough to drop the second lookup. -
apiImportsmoved relative to root modules in the generated entry. Old order waspageImports, apiImports, appImportCode, docImportCode; new order isdocImportCode, appImportCode, pageImports, apiImports. So API-route modules now evaluate after_document/_app/page instead of before. API routes are independent endpoints with no expected side-effect coupling to root modules, so this is inert in practice — noting only for completeness.
Nothing blocking. LGTM.
|
Review posted to PR #2560. SummaryReviewed at head Verified locally:
Correct behavior confirmed:
Two minor, non-blocking notes:
No blocking issues found. |
|
Independent Codex review — NO FINDINGS Reviewed exact head |
1 participant
Summary
_document→_app→ pageFailure addressed
From Actions run 28938793088:
test/e2e/app-document-import-order/app-document-import-order.test.ts—root components should be imported in order _document > _app > pageThe two
test/e2e/app-document/rendering.test.tsnonce/crossOrigin failures are already owned by #2044; I commented there with the exact failing tests and intentionally did not duplicate that work.Next.js oracle
Pinned Next.js
v16.3.0-canary.80at9dd6d677b63b249584c8ff0bccffcc6a65288683.Validation
vp test run tests/entry-templates.test.ts tests/pages-router.test.ts— 395 passedvp check packages/vinext/src/entries/pages-server-entry.ts packages/vinext/src/server/dev-server.ts tests/entry-templates.test.ts tests/pages-router.test.ts— passedgit diff --check— passedREPO="$(pwd)" NEXTJS_DIR="/Users/jamesanderson/Developer/vinext/.nextjs-ref-v16.3.0-canary.80" ./scripts/run-targeted-nextjs-e2e.sh test/e2e/app-document-import-order/app-document-import-order.test.ts— 1 passed, 1 upstream Turbopack skipNotes