Skip to content

fix(pages): clear fallback route params before hydration#2567

Open
james-elicx wants to merge 3 commits into
mainfrom
codex/fix-pages-fallback-route-params-28938793088
Open

fix(pages): clear fallback route params before hydration#2567
james-elicx wants to merge 3 commits into
mainfrom
codex/fix-pages-fallback-route-params-28938793088

Conversation

@james-elicx

Copy link
Copy Markdown
Member

Summary

  • clear Pages Router fallback-shell query state before _app, page, and _document rendering
  • serialize an empty __NEXT_DATA__.query for fallback shells across dev, production, and Worker request paths
  • keep the client router on the serialized empty query until the hydration _h replace fetches page data and commits the concrete route params

Next.js parity

Ported from Next.js v16.3.0-canary.80 (9dd6d677b63b249584c8ff0bccffcc6a65288683):

  • test/e2e/fallback-route-params/fallback-route-params.test.ts
  • packages/next/src/server/render.tsx — fallback renders clear query
  • packages/next/src/client/index.tsx — fallback hydration performs a non-shallow router replace

Validation

  • vp test run tests/pages-page-data.test.ts tests/pages-page-response.test.ts tests/pages-page-handler.test.ts — 131 passed
  • vp test run tests/pages-router.test.ts -t "fallback shell|fallback: true" — 6 passed
  • vp test run tests/shims.test.ts -t "keeps fallback route params empty until hydration data commits" — 1 passed
  • targeted vp check for all 9 touched files — clean
  • vp run vinext#build — passed
  • exact prepared Next.js deploy wrapper:
    • test/e2e/fallback-route-params/fallback-route-params.test.ts — 2 passed
    • skeleton __NEXT_DATA__.query is empty
    • initial hydrated render has no slug, then the data navigation commits { slug: "second" }

Run source: 28938793088.

@pkg-pr-new

pkg-pr-new Bot commented Jul 8, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2567
npm i https://pkg.pr.new/create-vinext-app@2567
npm i https://pkg.pr.new/vinext@2567

commit: 3a7ab0a

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor
Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original
workers-cache preview production
web preview production

@github-actions

github-actions Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared 3a7ab0a against base 617bffa using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 1 regressed · 5 within ±1.5%

Scenario Framework Baseline Current Change
Client bundle size (gzip) vinext 126.6 KB 126.6 KB ⚫ +0.0%
Client entry size (gzip) vinext 120.6 KB 120.6 KB ⚫ +0.0%
Dev server cold start vinext 2.95 s 3.01 s 🔴 +2.0%
Production build time vinext 3.10 s 3.07 s ⚫ -1.2%
RSC entry closure size (gzip) vinext 98.5 KB 98.5 KB ⚫ 0.0%
Server bundle size (gzip) vinext 165.1 KB 165.1 KB ⚫ -0.0%

View detailed results and traces

🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

UnknownError: "Overloaded"

github run

@ask-bonk

ask-bonk Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

@james-elicx Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

UnknownError: "Overloaded"

github run

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

UnknownError: "Overloaded"

github run

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

UnknownError: "Overloaded"

github run

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

/bigbonk review

Reviewed the fallback route-params query clearing. Verified behavior against Next.js canary (packages/next/src/server/render.tsx, base-server.ts, and test/e2e/fallback-route-params/fallback-route-params.test.ts). The change is correct and matches Next.js observable behavior in both dev and production. No blocking issues found.

What I verified

Next.js parity is accurate. The Next.js fallback-route-params E2E test asserts:

  • Skeleton HTML: expect(query).toEqual({}) — empty __NEXT_DATA__.query
  • First client render: expect(initialSlug).toBeFalsy()
  • After the fallback data navigation: expect(hydratedQuery).toEqual({ slug: 'second' })

That test runs in production (start) mode, not just dev, so the empty-query skeleton is the correct prod behavior too. Notably, Next.js reaches the empty query via two different internal mechanisms — a dev-only query = {} reset in render.tsx (gated on process.env.__NEXT_DEV_SERVER) vs. the production prerender/response-cache serving an empty-query shell — but the served output is identical (__NEXT_DATA__.query === {}). This PR producing {} in both modes is the right call.

Dev/prod parity is maintained. Both request paths clear the fallback render query consistently:

  • Dev: dev-server.tsrenderQuery is set to {} at the fallback branch (L936) and flows into setSSRContext (L940), _app.getInitialProps via loadAppInitialProps (L986), __NEXT_DATA__ (L1854), and documentContext (L1930). All usages are correctly ordered after isFallbackRender is determined.
  • Prod: pages-page-handler.ts renderQuery (L669) → renderPagesPageResponsebuildPagesNextDataScript (pages-page-response.ts L282). serializedPagesNextData carries no query field, so the nextData merge loop in buildPagesNextDataScript won't clobber the empty query. Prod uses this path via pages-server-entry.tscreatePagesPageHandler.

The client commits real params after hydration (the other half of the contract). pages-client-entry.ts (L297-305) and dev-server.ts (L1843-1845) fire Router.replace(..., { _h: 1 }) when nextData.isFallback, and buildPagesNavigationNextData (shims/router.ts L1954-1962) sets isFallback: false + concrete merged params. getPathnameAndQuery's nextData?.isFallback ? {} : ... gate (L1287) then yields the real query once the replace commits. The added shim test exercises this transition directly.

_app.getInitialProps gets an empty query for fallback (pages-page-data.ts L742: isFallback ? { ...options, query: {} } : options) — matches Next.js, where the fallback ctx.query is {}.

Notes (non-blocking)

  • The copySerializedRouteQuery extraction (shims/router.ts) is a clean no-op refactor of the previous inline loop — behavior preserved.
  • features.test.ts tightening (data.query now asserted toEqual({ id: "1" }) for a non-fallback gSP page) is a good correctness improvement and confirms the non-fallback path still serializes route params only.
  • No regression for non-fallback pages: documentContext.query = renderQuery (== full query when not fallback), and __NEXT_DATA__.query = params — both unchanged from pre-PR behavior.

Local validation

  • vp test run tests/pages-page-response.test.ts tests/pages-page-data.test.ts — 108 passed
  • vp test run tests/shims.test.ts -t "keeps fallback route params empty until hydration data commits" — 1 passed
  • vp test run tests/pages-router.test.ts -t "fallback shell|fallback: true" — 6 passed
  • vp test run tests/features.test.ts -t "..." — passed
  • vp test run tests/pages-page-handler.test.ts — 24 passed
  • vp check on all 5 touched source files — clean

LGTM.

@ask-bonk

ask-bonk Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2567.

Summary

I reviewed PR #2567 (fix(pages): clear fallback route params before hydration) for issues. No blocking problems found — the change is correct and matches Next.js.

Key findings

  1. Next.js parity is accurate. I verified against canary render.tsx, base-server.ts, and the fallback-route-params E2E test. Empty __NEXT_DATA__.query for fallback shells is the correct observable behavior in both dev and production — the E2E test runs in production mode and asserts query === {} in the skeleton, then { slug: "second" } after the client data navigation.

  2. Dev/prod parity is maintained. Both the dev path (dev-server.ts renderQuery) and the prod path (pages-page-handler.tspages-page-response.ts buildPagesNextDataScript) clear the fallback query consistently. I confirmed serializedPagesNextData carries no query field, so the merge loop can't clobber the empty query.

  3. The client commits real params post-hydration via the existing _h replace flow (buildPagesNavigationNextData sets isFallback: false + concrete params), satisfying the full Next.js contract.

  4. No regression for non-fallback pages — the documentContext.query / __NEXT_DATA__.query behavior is unchanged from before the PR.

Validation

Ran all relevant targeted tests locally (pages-page-response, pages-page-data, shims fallback test, pages-router fallback tests, features, pages-page-handler) — all passing — plus vp check on the 5 touched source files (clean).

github run

@james-elicx
james-elicx marked this pull request as ready for review July 8, 2026 18:34
@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

/bigbonk review

Reviewed the fallback route-params query-clearing change against Next.js canary. The core change is correct and matches Next.js observable behavior in dev, prod, and Worker paths. No blocking issues. One minor, non-blocking parity note below.

What I verified

The empty-query reset is NOT dev-only in Next.js. packages/next/src/server/render.tsx L638–646 resets query = {} for isAutoExport || isFallback with no process.env.__NEXT_DEV_SERVER guard — render.tsx runs in both dev and prod. So producing an empty __NEXT_DATA__.query in both modes (this PR) is the correct behavior, and the E2E test (test/e2e/fallback-route-params/fallback-route-params.test.ts) runs in production start mode asserting query === {} in the skeleton.

No query clobber in the __NEXT_DATA__ merge. buildPagesNextDataScript sets query on L282 then merges options.nextData (L287–293), which would overwrite query if present. Confirmed serializedPagesNextData derives from buildPagesReadinessNextData (pages-readiness.ts), whose return type is gssp|gsp|gip|appGip|autoExport|__vinextno query field — so the empty query survives the merge. Correct.

Dev/prod/Worker parity holds. Prod and Worker both route through pages-server-entry.tscreatePagesPageHandler, where renderQuery = isFallbackRender ? {} : query (L669) flows into setSSRContext, renderPagesPageResponse, and __NEXT_DATA__. Dev (dev-server.ts) mirrors this with its own renderQuery (L936, L940, L986, L1854, L1927). All three paths are covered.

_app.getInitialProps receives {} for fallback (pages-page-data.ts L742: isFallback ? { ...options, query: {} } : options), matching Next.js ctx.query === {}. Verified the background ISR regeneration call at L903 correctly uses full options.query — that path regenerates the concrete (non-fallback) page, so it must keep real params.

Client commits real params post-hydration. The _h non-shallow replace (pages-client-entry.ts L297–305, dev-server.ts L1843–1845) preserves location.search and triggers buildPagesNavigationNextData (shims/router.ts L1954–1962), which sets isFallback: false + concrete mergedQuery. The getPathnameAndQuery gate (nextData?.isFallback ? {}, L1287) then yields real params. Matches Next.js createRouter(initialData.page, initialData.query={}, ...) → replace flow.

copySerializedRouteQuery is a faithful no-op extraction of the prior inline loop (same string/array/undefined-drop semantics). features.test.ts tightening (data.querytoEqual({ id: "1" })) is a good correctness improvement confirming non-fallback pages still serialize route params only.

Non-blocking note

  • asPath diverges from Next.js during the fallback shell. Next.js render.tsx L642 sets asPath = pathname (the route pattern) for isAutoExport || isFallback; vinext keeps requestAsPath/renderAsPath (the concrete request URL) in the fallback setSSRContext (dev-server.ts L941, pages-page-handler.ts L678). This is self-consistent (server and pre-replace client both compute the concrete path, so no hydration mismatch) and the E2E test doesn't assert asPath, so it isn't a regression. But a component reading router.asPath in a fallback shell will observe /second under vinext vs /[slug] under Next.js. Worth a follow-up if strict asPath parity is desired.

Validation

  • tests/pages-page-response.test.ts + tests/shims.test.ts -t "fallback" — 21 passed
  • tests/pages-router.test.ts -t "fallback" — 18 passed

LGTM.

@ask-bonk

ask-bonk Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2567.

Summary

I reviewed PR #2567 (fix(pages): clear fallback route params before hydration) against Next.js canary source. The change is correct across dev, prod, and Worker paths. No blocking issues. One minor non-blocking parity note.

Key findings

  1. Empty-query reset is correct in both modes. I verified render.tsx L638–646 resets query = {} for fallback with no dev-server guard, so producing an empty __NEXT_DATA__.query in both dev and prod is right. (An earlier Bonk review had claimed this reset was dev-only — that's inaccurate for this specific reset.)

  2. No query clobber. Confirmed serializedPagesNextData (from buildPagesReadinessNextData) has no query field, so the nextData merge loop in buildPagesNextDataScript can't overwrite the empty query.

  3. Dev/prod/Worker parity holds. Prod + Worker share createPagesPageHandler; dev mirrors it. All three set renderQuery = {} for fallback consistently.

  4. Client commits real params post-hydration via the _h non-shallow replace → buildPagesNavigationNextData (isFallback: false + concrete params), and the background ISR regeneration path correctly keeps full query.

  5. Non-blocking note: asPath diverges from Next.js during the fallback shell (vinext keeps the concrete request URL; Next.js uses the route pattern). It's self-consistent (no hydration mismatch) and untested by the E2E suite, but a component reading router.asPath in a fallback shell would observe /second vs Next.js's /[slug]. Worth a follow-up only if strict parity is desired.

Validated with the fallback-focused test files (39 tests passing).

github run

@james-elicx

Copy link
Copy Markdown
Member Author

Independent Codex review — NO FINDINGS

Reviewed exact head 3a7ab0af3188f440b94f2ccfee3eac97f4a2313e. Scope: fallback dynamic-route skeleton/query semantics and hydration timing, including clearing fallback params before hydration data commits while preserving resolved route params afterward. Targeted Next.js v16.3.0-canary.80 oracle: test/e2e/fallback-route-params/fallback-route-params.test.ts2 passed; skeleton __NEXT_DATA__.query confirmed empty.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant