fix(pages): pass AppTree to page initial props#2562
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 |
There was a problem hiding this comment.
Review: fix(pages): pass AppTree to page initial props
Reviewed against Next.js v16.3.0-canary.80. The change is correct and well-scoped. Verified behavior against the Next.js source and ran the relevant test files locally — all green.
Correctness verification
Nested ctx.AppTree matches Next.js. Next.js sets ctx.AppTree = AppTree in Router.getInitialProps (shared/lib/router/router.ts:2630) and duplicates it into the nested ctx in client/index.tsx:411. The PR mirrors this in every path: shims/router.ts (ctx: { ...ctx, AppTree } and PageComponent.getInitialProps({ ...ctx, AppTree })), pages-get-initial-props.ts (ctx.AppTree), and pages-page-data.ts. Good parity.
committedComponent correctly uses the previous page during navigation. In loadComponentOnlyProps, the client AppTree closure binds committedComponent (the last committed page), not the incoming PageComponent. This matches Next.js exactly: _wrapApp uses the module-level CachedComponent (client/index.tsx:343), which is only updated in render() after getInitialProps(routeInfo.Component, ctx) runs. So during a navigation's data phase, AppTree renders the cached/old component while getInitialProps still receives the new Component separately — the PR replicates this precisely. The precedence committedComponent ?? window.__VINEXT_PAGE_COMPONENT__ ?? PageComponent also correctly handles the first-navigation-after-hydration case, since __VINEXT_PAGE_COMPONENT__ is set at hydration in both the client entry and the dev hydrate template.
Dev error page now runs App.getInitialProps. Previously renderErrorPage only ran ErrorComponent.getInitialProps and never invoked the custom _app's data function. The new branch runs App.getInitialProps (which by convention calls Component.getInitialProps(ctx)) when present, matching Next.js client/index.tsx:411 (loadGetInitialProps(App, appCtx)). statusCode is still force-merged afterward, so the default _error statusCode contract is preserved. Prod already handled this via resolvePagesPageData, so dev/prod parity is now aligned.
Prod path parity holds. pages-page-handler.ts wires createAppTree to createPageElement(PageComponent, AppComponent, ...), and pages-server-entry.ts:createPageElement wraps with the App component when present — so createAppTree builds the full App tree, not a bare page. The ?? options.createPageElement fallback in pages-page-data.ts is therefore safe (prod always supplies createAppTree; the fallback still produces a valid element).
Test coverage
Coverage is appropriately focused:
shims.test.ts: two new tests exercising the committed-component AppTree through both custom-_appand page-onlygetInitialPropsduring no-data navigation, including a second navigation to confirm the snapshot advances.pages-router.test.ts: route-aware AppTree for default-_apppage GIP, plus dev error rendering running custom_app.getInitialPropswith nested AppTree.- The ISR-regen
appTreeWrapCountchange 1 → 2 is expected (the new fixture'sPage.getInitialPropsnow invokes AppTree, adding one wrap on top of the final render wrap).
Local validation
vitest run tests/pages-page-data.test.ts tests/pages-get-initial-props.test.ts— 64 passedvitest run tests/shims.test.ts -t AppTree— 2 passedvitest run tests/pages-router.test.ts -t "AppTree|error|ISR|regeneration"— all passedtsc --noEmit -p packages/vinext/tsconfig.json— clean
Nits / non-blocking
- Minor duplication. The AppTree-closure construction pattern (App? wrap-with-App : bare-component, then
wrapWithRouterContext) is now hand-rolled in four places:shims/router.ts(loadComponentOnlyProps),dev-server.ts(createAppTreeandcreateErrorAppTree), andpages-page-handler.ts. These are close but not identical (error path passeserrorRouter, client path passescommittedSnapshot). Not worth blocking on, but a shared helper could reduce drift risk in future changes, consistent with the AGENTS.md guidance to keep runtime logic in typedserver/*helpers. - The excluded
no-page-propsbacklog item is reasonably justified in the PR description — its_app404-visibility failures aren't demonstrably caused by the missing nested AppTree.
No correctness issues found. LGTM.
|
Review posted to PR #2562. SummaryI reviewed PR #2562 ( Key findings: Correct parity with Next.js:
Test coverage is focused and appropriate; the ISR Local validation: all targeted test files pass ( Non-blocking nits: flagged minor duplication of the AppTree-construction pattern across four sites ( |
Summary
AppTreerenderer through the nestedNextPageContextsupplied to_app.getInitialProps_appforwardingctxtoPage.getInitialPropsRoot cause
Next.js includes
AppTreeboth on the outerAppContextand the nestedctxpassed to_app.getInitialProps. Vinext only populated the outer field. Custom apps that callComponent.getInitialProps(ctx)therefore forwarded a context withoutAppTree, causing page-level rendering throughAppTreeto throw.Next.js parity
Fixes run 28938793088:
test/e2e/app-tree/app-tree.test.ts—should pass AppTree to NextPageContextOracle: Next.js
v16.3.0-canary.80at9dd6d677b63b249584c8ff0bccffcc6a65288683.Validation
vp test run tests/pages-page-data.test.ts tests/pages-get-initial-props.test.ts— 63 passedvp check packages/vinext/src/server/pages-page-data.ts packages/vinext/src/server/pages-get-initial-props.ts tests/pages-page-data.test.ts tests/pages-get-initial-props.test.ts— cleanvp run vinext#build— passedNEXTJS_DIR=/Users/jamesanderson/Developer/vinext/.nextjs-ref-v16.3.0-canary.80 VINEXT_BUILD=0 REPO=$PWD ./scripts/run-targeted-nextjs-e2e.sh test/e2e/app-tree/app-tree.test.ts— 3 passedSeparate backlog
test/e2e/no-page-props/no-page-props.test.tsremains queued separately. Its custom_app404 visibility failures are not demonstrably caused by the missing nestedAppTree, so they are intentionally excluded from this PR.Bonk has not been requested yet.