fix(pages): serialize full merged query into SSR __NEXT_DATA__.query (#1970)#2060
fix(pages): serialize full merged query into SSR __NEXT_DATA__.query (#1970)#2060Divkix wants to merge 3 commits into
Conversation
commit: |
|
/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): serialize full merged query into SSR __NEXT_DATA__.query
Verified the fix against the codebase and Next.js behavior. This looks correct and well-tested. I checked out the branch and ran the relevant tests; all 8 new #1970 integration tests pass, the 5 unit tests pass, the getStaticProps carve-out regression guard in features.test.ts stays green, and vp check is clean on all 5 changed source files.
What I verified
Core claim holds. getRouteQueryFromNextData in shims/router.ts:1201 only reads route params from __NEXT_DATA__.query when the visible URL path does not match __NEXT_DATA__.page (the rewrite case at line 1208). When the path matches, params come from the path and the querystring from location.search. So adding the querystring (gSSP) does not affect useRouter().query — the change is purely a direct-reader fix, as described.
The autoExport/fallback carve-out keeping params (not {}) is correct and necessary. The rewrite-navigation/[id]/destination.tsx fixture is autoExport, and /rewrite-navigation/0 rewrites to a path that doesn't contain id, so the client recovers id from __NEXT_DATA__.query. Resetting to {} (Next.js behavior) would drop it. The new regression test guards exactly this. Good catch documenting the intentional divergence.
SSR/soft-nav parity confirmed. All SSR sites pass mergeRouteParamsIntoQuery(parseQuery(url), params) as query (dev-server.ts:579, pages-page-handler.ts:421), matching the client soft-nav value at shims/router.ts:1815 — including route-params-win-on-collision (the /posts/42?id=evil → {id:"42"} test).
All four serialization sites covered. dev SSR (dev-server.ts:1662), dev ISR regen (dev-server.ts:1233), and the shared buildPagesNextDataScript (pages-page-response.ts:303, used by prod render + prod ISR regen). No query: params site was missed.
ISR safety confirmed. renderPagesIsrHtml is only reached for the gsp path, where computePagesNextDataQuery returns params regardless of query, so no request-specific querystring leaks into cached HTML. The path-only cache key is fine.
Minor nits (non-blocking)
-
PR description says "helper unit test: 6/6 passing" but
tests/pages-readiness.test.tscontains only 5 tests (the gSSP and gIP cases are identical inputs/duplicate assertions). The integration block adds 8 tests, not 6. Just a count discrepancy in the description — no code impact. -
The gIP unit test duplicates the gSSP unit test exactly (same inputs). Since the helper only distinguishes on
isFallback/autoExport/gsp, a gIP page is structurally identical to gSSP at the helper level. The integration test (nav-compat-gip) is what actually exercises the gIP classification path, so this is fine — just noting the unit-level duplication is cosmetic.
No blocking issues. Nice, surgical fix with thorough test coverage and clear documentation of the Next.js parity reasoning.
| ).toEqual({ id: "42", q: "foo" }); | ||
| }); | ||
|
|
||
| it("returns the full merged query for getInitialProps pages", () => { |
There was a problem hiding this comment.
Nit: this case has identical inputs to the gSSP test above (gsp: undefined, autoExport: false), so it doesn't exercise a distinct branch in computePagesNextDataQuery — gIP classification only matters upstream in buildPagesReadinessNextData. The integration test against the nav-compat-gip fixture is what actually covers the gIP path. Harmless, just noting the unit-level redundancy.
|
Review posted successfully to PR #2060. SummaryI reviewed PR #2060 ( Verification performed
Minor nits raised (non-blocking)
No blocking issues — a surgical, well-documented fix. |
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
|
Addressed the two nits from the review: removed the duplicate gIP unit test (identical to gSSP case — real gIP coverage is in the integration test), and fixed the PR description counts (5/5 unit tests, 8 integration tests). |
…loudflare#1970) SSR-rendered Pages Router pages serialized __NEXT_DATA__.query with only dynamic route params, dropping the URL querystring. This diverged from Next.js and from vinext's own client router, which writes the full merged query (querystring + route params) into __NEXT_DATA__.query after a soft navigation — so direct readers of __NEXT_DATA__.query saw a different shape on initial SSR vs. after a soft nav back to the same URL. Add a shared computePagesNextDataQuery helper in pages-readiness.ts that reproduces Next.js's serialization carve-out, and use it at all four SSR serialization sites (prod render + prod ISR regen in pages-page-response.ts / pages-page-data.ts, and dev SSR + dev ISR regen in dev-server.ts): - getServerSideProps / getInitialProps -> full merged query - getStaticProps (non-fallback) -> route params only (querystring dropped) - autoExport / getStaticPaths fallback -> {} useRouter().query is unaffected: the client recovers route params from the URL path and the querystring from location.search, independent of the serialized __NEXT_DATA__.query content — so the change only fixes direct readers, with no hydration impact.
…xport renders The initial cloudflare#1970 change reset __NEXT_DATA__.query to {} for autoExport and fallback renders (Next.js parity). That broke client param recovery for rewritten dynamic routes: vinext's client reads a dynamic route's params from __NEXT_DATA__.query whenever the visible URL path doesn't match the route pattern (e.g. the rewrite /rewrite-navigation/0 -> /rewrite-navigation/[id]/ destination), because the params live nowhere else on the page. Emptying the query dropped useRouter().query.id on hydration — caught by the pages-router navigation e2e (initial load and query-only navigations). Narrow the carve-out so only per-request server renders (getServerSideProps / getInitialProps / _app.getInitialProps) serialize the full merged query — the actual cloudflare#1970 fix. getStaticProps, autoExport, and fallback keep route params only (the original behavior), preserving rewrite param recovery while still dropping the querystring from static renders. Add an integration regression test asserting the rewritten autoExport route's __NEXT_DATA__.query retains its route param, and update the helper/integration tests to the corrected semantics.
Divkix
force-pushed
the
fix/issue-1970-next-data-query
branch
from
8e5ba0f to
11a6961
Compare
2 participants
Fixes #1970.
Problem
For SSR-rendered Pages Router pages with a querystring (notably
getServerSidePropspages like/search?q=foo), the inlinedwindow.__NEXT_DATA__.querycontained only dynamic route params and dropped the URL querystring. This:__NEXT_DATA__.queryfor gSSP/gIP pages.__NEXT_DATA__.querywith the full merged query after a soft navigation (shims/router.ts, with a comment noting direct readers would otherwise see only dynamic params). So anything reading__NEXT_DATA__.querydirectly (test harnesses, analytics, third-party scripts) saw one shape on initial SSR and a different shape after a soft nav back to the same URL.useRouter().querywas already correct (the client recovers route params from the path and the querystring fromlocation.search), so this was purely a__NEXT_DATA__.queryserialization bug.Fix
Add a shared
computePagesNextDataQueryhelper inpages-readiness.tsthat reproduces Next.js's serialization carve-out, and use it at all four SSR serialization sites (prod render + prod ISR regen inpages-page-response.ts/pages-page-data.ts, and dev SSR + dev ISR regen indev-server.ts):__NEXT_DATA__.querygetServerSideProps/ pagegetInitialProps/_app.getInitialPropsgetStaticProps(non-fallback render)getStaticPathsfallback shell{}The carve-out is required, not cosmetic: a naive "always use the full query" change would regress the existing assertion that
getStaticPropspages drop the querystring (tests/features.test.ts).Why there's no hydration impact
shims/router.tsgetRouteQueryFromNextDatarecovers dynamic route params foruseRouter().queryby matchingwindow.location.pathnameagainst__NEXT_DATA__.page(the route pattern), not from the content of__NEXT_DATA__.query;getPathnameAndQuerythen mergeslocation.search. So adding the querystring (gSSP) or resetting to{}(autoExport dynamic) does not affectuseRouter().query— the change only fixes direct readers of__NEXT_DATA__.query.ISR note: HTML ISR caching is gated to the
getStaticPropspath (gSSP responses are never written to the HTML cache, and cached gsp queries are querystring-independent), so the existing path-only cache key needs no change.Tests
tests/pages-readiness.test.tsunit-tests the helper across all branches (gSSP→full query, gsp→params, autoExport→{}, isFallback→{}, isFallback overriding gsp).tests/pages-router.test.ts(router __NEXT_DATA__ correctnessblock) cover gSSP querystring inclusion, route-param/querystring collision, array querystring values, getStaticProps querystring drop, autoExport-dynamic reset, and gIP full query — all reusing existingpages-basicfixtures.tests/features.test.tsgetStaticProps carve-out and the prior top-levelnextData.queryassertions.Verification
pages-router.test.ts: 319/319 passing ·features.test.ts: 327/327 passing · helper unit test: 5/5 passing.