fix(app-router): build client page searchParams in SSR and the browser - #3457
james-elicx wants to merge 22 commits into
Conversation
|
/bigbonk review for issues Please review exact head |
@vinext/cloudflare
create-vinext-app
@vinext/types
vinext
@cloudflare/workers-response-store
commit: |
Performance benchmarksCompared 0 improved · 1 regressed · 5 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| ): Promise<ClientPageSearchParams> { | ||
| const record = searchParamsToRecord(searchParams); | ||
| const promise = Promise.resolve(record) as SettledClientPageSearchParams; | ||
| promise.status = "fulfilled"; |
There was a problem hiding this comment.
[P2] Keep React bookkeeping out of synchronous enumeration. These assignments create enumerable own status and value properties, while the SSR makeThenableParams proxy filters those reserved keys from ownKeys. A client page that synchronously does Object.keys(searchParams) or { ...searchParams } therefore renders only query keys during SSR but sees status/value in the browser, causing a hydration mismatch and exposing implementation fields. Define these bookkeeping properties as non-enumerable (and add an enumeration/hydration test).
There was a problem hiding this comment.
Fixed in 6eb65c6. React's status and value on the browser promise are now non-enumerable, so Object.keys, Object.entries and spreading list the same query keys as the SSR thenable. The promise also resolves to a plain object with Object.prototype, as in SSR. Tests compare enumeration against the SSR thenable, including reserved keys.
| getNavigationContext()?.clientPageSearchParams ?? createClientPageSearchParams(null); | ||
| } else { | ||
| const urlSearchParams = useSearchParams(); | ||
| searchParams = useMemo(() => createClientPageSearchParams(urlSearchParams), [urlSearchParams]); |
There was a problem hiding this comment.
[P2] Preserve force-static semantics during browser navigation. This always reconstructs the prop from useSearchParams(), so a client page under dynamic = "force-static" can observe a destination query during a Link/router transition even though SSR supplies an empty object and Next.js keeps searchParams empty. The existing force-static hydration test only covers useSearchParams() on a hard load, not the page prop across client navigation; carry the force-static policy into this branch and cover that transition.
There was a problem hiding this comment.
Fixed in be532d9. Dispatch (the main page, the intercept source and revalidation) and server-action rerenders now pass isForceStatic to the element builder. A force-static client page then gets an emptySearchParams prop, so SSR and the browser both give it an empty, untracked query, across client navigations too. Static export production builds get the same treatment. Covered by unit tests and a new e2e with a force-static client page doing a Link transition.
|
Posted 2 actionable inline findings. Targeted tests passed: 157/157. |
7b4fee6 to
1d0047c
Compare
1d0047c to
0e66e0f
Compare
0e66e0f to
90fb1a2
Compare
|
/bigbonk review for issues Please review exact head |
|
| defineHiddenProperty(promise, "value", record); | ||
| for (const key of Object.keys(record)) { | ||
| if (isWellKnownProperty(key)) continue; | ||
| Reflect.defineProperty(promise, key, { |
There was a problem hiding this comment.
[P2] Keep direct own-property checks consistent with SSR. On the server, makeThenableParams binds hasOwnProperty/propertyIsEnumerable to the underlying Promise target, where the virtual query keys do not exist; here the browser promise materializes those keys as own properties. With ?q=x, searchParams.hasOwnProperty("q") therefore renders false during SSR (without recording the read) and true during hydration. Please make these Object-prototype methods agree across both implementations and cover the direct-call case.
There was a problem hiding this comment.
Fixed in e47396f. makeThenableParams now binds hasOwnProperty and propertyIsEnumerable to the proxy rather than the Promise target, so they go through its traps. They now answer like the browser record and mark the read. Tests cover the browser and SSR agreeing on a query key, a missing key and the reserved then, and the SSR call marking the render dynamic.
| } else { | ||
| // Every App Router SSR render sets this. Without it there is no query this | ||
| // render may safely read, so the page gets an empty one. | ||
| searchParams = |
There was a problem hiding this comment.
[P2] Avoid deciding the hydration query before a delayed client-page read can mark the render dynamic. The head bootstrap snapshots searchParamsGate.gate.decision; if a client page behind Suspense reads this thenable after the head was emitted, SSR uses the effective rewritten query but the bootstrap has already told the browser to recover the public URL query. A rewrite such as /alias -> /page?q=rewritten then hydrates different text. The query-source decision needs to remain correct for dynamism discovered after head emission, with a delayed-boundary rewrite hydration test.
There was a problem hiding this comment.
Fixed in 28287d5. After the gate settles, SSR keeps listening for the render turning dynamic. If the head hid the query and the render turns dynamic afterwards, the next flush sends the query client pages used, ahead of their HTML. That render can't be stored, so the document may carry it. The browser's hydration snapshot reads that value when a client page first renders. The new e2e "a client page behind a delayed boundary hydrates with the rewritten query" in static-hydration.spec.ts fails without the fix (hydration error #418).
|
Posted 2 inline findings. Additional findings:
Targeted tests passed: 184/184. |
90fb1a2 to
8fcfc34
Compare
|
The two additional findings from the last review:
|
|
/bigbonk review for issues Please review exact head |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues Please review exact head |
8fcfc34 to
0242802
Compare
|
/bigbonk review for issues Please review exact head |
| actionInitiation.href, | ||
| actionInitiation.routerState.navigationSnapshot.params, | ||
| // The re-render keeps the URL, so the server rendered the same query. | ||
| const navigationSnapshot = withRenderedSearchOf( |
There was a problem hiding this comment.
[P1] Use the rendered query from the action response here. A same-URL action can still be rendered through a different effective query (for example a conditional middleware/config rewrite on the POST), and the response already carries X-Vinext-Rendered-Path-And-Search. Copying the pre-action snapshot makes ClientPageRoot in the fresh Flight tree read the old query, potentially mismatching the tree the server just rendered. Please parse that header for non-redirect action results and thread it into this snapshot too.
There was a problem hiding this comment.
Fixed in 63f22e9. The action re-render now sends X-Vinext-Rendered-Path-And-Search, and the client threads it into the same-URL snapshot. Covered by an e2e with a rewrite that only applies to actions.
| createClientNavigationRenderSnapshot( | ||
| window.location.href, | ||
| latestClientParams, | ||
| getHydrationRenderedPathAndSearch(), |
There was a problem hiding this comment.
[P2] Preserve the rendered query from the fetched response in the no-embedded-RSC fallback. In that branch readInitialRscStream() fetches a fresh RSC response but restores navigation context from window.location, ignoring X-Vinext-Rendered-Path-And-Search; this call therefore records the public query. For /alias -> /page?q=rewritten, the fallback tree gives its client page the alias query instead of the query used by the server. Parse the fallback response header before constructing this initial snapshot.
There was a problem hiding this comment.
Fixed in 8c68155. The fetched-payload fallback now restores the navigation context from the response's rendered-path header.
| window.location.href, | ||
| latestClientParams, | ||
| // Same URL, so the same rendered query. | ||
| const navigationSnapshot = withRenderedSearchOf( |
There was a problem hiding this comment.
[P3] Derive this value from the fresh HMR response rather than the previous tree. A conditional rewrite can resolve the same browser URL to a different query between requests, and HMR then installs newly rendered client-page props while this copied snapshot still supplies the old query. Keep the response available long enough to parse X-Vinext-Rendered-Path-And-Search, as the normal navigation path does.
There was a problem hiding this comment.
Fixed in b3ae004. HMR now waits for the response headers and builds the snapshot from them. It also drops the update if a newer one started in the meantime.
| // promise's species, so a query key of that name would break a real | ||
| // promise that carries the keys as own properties (the browser's client | ||
| // page searchParams). | ||
| "constructor", |
There was a problem hiding this comment.
[P2] Do not add constructor to the shared reservation used by makeThenableParams. This changes existing server-page behavior for valid keys: a [constructor] segment or ?constructor=value now returns the Promise constructor from synchronous params.constructor/searchParams.constructor instead of the route value (while await params still returns the value). The Next.js well-known set also does not reserve this key. If the browser client-page promise needs it protected, use a browser-specific predicate rather than changing all params.
There was a problem hiding this comment.
Fixed in a297b2d. constructor is out of the shared list, which matches Next.js's wellKnownProperties. The browser client-page promise is now a Proxy over a real promise, so a constructor query key reads synchronously like the SSR thenable.
|
Posted 4 actionable inline findings. Targeted tests passed: 47/47. Branch was not modified. |
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
…rved keys Move the URLSearchParams-to-record loop into utils/query.ts and the Promise/React reserved property list into a module free of server-only imports, so browser code can build page searchParams the same way.
A client page no longer receives searchParams through Flight, like Next.js's ClientPageRoot. Flight called `then` on the prop while serializing it, so every client page was observed reading the query and its RSC payload carried it. - A "use client" ClientPageRoot shim renders the page. In SSR it reads a per-render thenable from the navigation context; a read marks the render dynamic, so a page that reads the query is never stored and one that doesn't stays cacheable. force-static and PPR fallback shells keep an untracked query. - In the browser it builds a settled promise from useSearchParams(), with the query keys readable synchronously. - Slot client pages drop the route searchParams from their Flight props.
… without a query The markDynamic: hasRequestSearchParams exemption existed for client pages, whose searchParams Flight always awaited. Class components were only covered because isReactOwnedAppComponent groups them with client references. Flight hands a class its props instead of serializing them, so a read is a real read and follows function component pages.
…namic The markDynamic option existed for the query-only exemption, which the previous commit removed, so every caller now marks the render dynamic. Also correct the class component rationale: React 19's Flight server calls any function that isn't a client reference as a function component, so an ES class page can't render in RSC. The branch only stays consistent with function component pages.
…thenable A page can tell the two apart during hydration, so they must agree: - React's status and value on the browser promise are non-enumerable, so Object.keys() and spreading list the same query keys as the SSR proxy. - The promise resolves to a plain object, so hasOwnProperty() works as in SSR. Spreading keeps a __proto__ key an own entry. - constructor joins the reserved names. As an own property of a real promise it replaced Promise#constructor, and await threw.
…chParams empty
A client page's searchParams no longer travel through Flight, so the
browser didn't know the server renders force-static pages with an empty
query. A Link or router transition showed the destination query, where
Next.js keeps it empty.
- Dispatch and server action rerenders tell the page builder when a
route is force-static. ClientPageRoot gets emptySearchParams, which is
query-free, and hands the page an empty, untracked query in SSR and
the browser.
- A static export build renders each page once without a query, and a
client page read marked it dynamic, so the prerender skipped it and the
export silently dropped it. Export builds get the same empty query, as
before client pages stopped reading searchParams from Flight.
- The SSR thenable helper takes the render's force-static and PPR
fallback shell state instead of a precomputed flag.
- Test a synchronous SSR read that is never stored, and a force-static
page that reads and is stored with {}.
…at rendered them The browser built a client page's searchParams from useSearchParams(), so it followed the live URL: - A rewritten query was lost. A soft navigation to /search/bar, rewritten to /search?q=bar, gave the page q undefined. - A page that stays mounted followed later URLs. Opening an intercepted modal changed the background page's query, and so did a kept parallel slot. Next.js reads the page's searchParams from its own segment payload. The payload is query-free here, so the navigation snapshot now carries the query the server rendered: X-Vinext-Rendered-Path-And-Search for fetched and cached navigations, the embedded navigation payload on hydration, and the current one for same-URL rerenders. ClientPageRoot captures it the first time it renders a server output, keyed by the page's props object, which Flight builds per server render and the router keeps with the segment. A kept page no longer re-renders on each navigation.
…namic in the probe and prerender SSR runs in a child scope of the render, so a client page's searchParams read there reaches the request's dynamic latch but not the render's own flag. Only candidate HTML renders read the latch, and neither the deploy probe (never a candidate) nor the build prerender is one, so both called the page static. Workers Cache then listed it as static-candidate and admission answered its first render with the static-to-dynamic 500. - Read the latch in every HTML render that decides whether the page is static: candidates, the Worker's probe and admission, and the build prerender, except PPR fallback shells. - Let a speculative prerender wait for SSR to finish before its headers, stopping as soon as the render turns dynamic, so a read after the shell inside Suspense counts too.
…s like the browser
…edirect target's render
…namic after the head
… rendered a refreshed kept branch
…r action re-render was rewritten to
…tched initial Flight payload
…re-render was rewritten to
… its response arrives
…wser searchParams The browser promise came with `status` and `value` already set, where the SSR thenable only gets them once React tracks it. A client page reading `searchParams.status` directly rendered `undefined` in SSR and `fulfilled` in hydration, a mismatch. Next.js's browser and SSR promises are both a plain `Promise.resolve(query)`, so neither carries them before `use()`. - The browser promise no longer sets them, so `use()` tracks it as React does any promise. - Test a direct read of both fields in SSR and hydration, as a unit render and in the production fixture. Browser unit renders wait for suspended pages.
…Components With Cache Components, Next.js builds a client page's searchParams from SearchParamsContext, the query useSearchParams() reads, not the one its segment payload rendered (client/components/client-page.tsx). The browser wrapper always read the rendered query, so a rewrite like /feed/bar -> /feed?tab=bar exposed tab=bar, and a kept page kept its old query when the URL changed. - In that mode the browser wrapper reads useSearchParams(), so the page sees the public query and a kept page follows the URL. force-static and static export pages still read an empty query. - Test a rewrite and a kept page with Cache Components.
…Flight payload Without an embedded payload, hydration took both the pathname and the query from `X-Vinext-Rendered-Path-And-Search`. That header names the rewrite target, so for /alias -> /page?q=rewritten usePathname() hydrated as /page where SSR rendered /alias, a mismatch. The RSC handler keeps the canonical pathname and the resolved query apart, as the embedded payload does. - Hydrate with the browser's pathname and only the header's query. - Test the location it resolves, and usePathname() in the production fixture's fetched-payload hydration.
Every client page in an SSR render received the same promise. React writes `status` and `value` onto a promise when a page calls use() on it, so a sibling reading those fields rendered them in SSR, while the browser builds one promise per page and the sibling hydrated with `undefined`. Next.js keys its promise by the page's server-provided query object, in SSR and the browser alike (`makeUntrackedSearchParams`), so both sides decode the same payload into the same promises. The browser wrapper here keys by the page's props object, so SSR now does too. - The SSR navigation context carries a per-render source that caches one promise per page props object, and drops it with the render. - Test a page using its query beside one reading the fields, as a unit render and in the production fixture.
f263a04 to
4b18cd0
Compare
|
/bigbonk review for issues Please review exact head |
|
LGTM! |
1 participant
Stacked on #3455. Plan PR 2: client page
searchParamsare built on the SSR and browser side, like Next.js'sClientPageRoot(client/components/client-page.tsx).Problem
A client page got
searchParamsthrough Flight. Flight calledthenon the prop while serializing it, so:A static client page that never touches
searchParamswas therefore never provably query-free. #3456's safety net would stop storing it. A client page that does readsearchParamswith no query in the URL was stored, which Next.js never does.Change
server/app-page-element-builder.ts): when the request hassearchParamsand the page is a client reference, the builder rendersClientPageRootwith{ Component, pageProps }.searchParamsisn't in those props, so Flight never touches it and the RSC payload is query-free. Slot client pages drop the route'ssearchParamsfrom their props. Boundary renders with nosearchParamsrender the client page directly, as before.shims/client-page-root.tsx("use client"):handleSsrbuilds this withmakeClientPageSsrSearchParamsThenable. Any read, including a synchronous property read, goes through the same observer as a server page's: it marks the render dynamic, records thesearchParamsusage, and throws fordynamic = "error". So a client page that reads the query is never stored, and one that doesn't stays cacheable.force-staticand PPR fallback shells get an untracked thenable instead, so their output doesn't change.useSearchParams(). The query keys are copied onto it for synchronous reads, except the names Promise and React rely on.markDynamic: hasRequestSearchParamsexemption was only there because Flight always awaited client page props (fbf68d5). Class components were caught by it only becauseisReactOwnedAppComponentgroups them with client references.collectAppPageSearchParamsnow usessearchParamsToRecord(utils/query.ts).shims/internal/thenable-well-known-properties.ts, so the browser shim can use them.Tests
tests/client-page-root.test.ts, with the real headers module:useSearchParams()gate and latches dynamic;tests/app-page-element-builder.test.ts:ClientPageRootprops have nosearchParams, the query doesn't appear, and nothing is observed;tests/app-page-render.test.ts: a candidate client page that doesn't read is written to the cache; one that reads withuse()in SSR isn't.examples/response-store-demo, new/client-search-params/*routes):tests/e2e/app-router-prod/static-hydration.spec.ts:?q=hellohydrates, a Link to?q=worldupdates the value, and there are no console errors.Known differences and residuals
useSearchParams()hydration. SSR uses the effective query.force-staticand browser-only reads: the browser prop shows the URL's query. Next.js gives{}from its static payload.