fix(pages): preserve app props for GSSP requests#1996
Conversation
commit: |
NathanDrake2406
force-pushed
the
nathan/gssp-upstream-block
branch
3 times, most recently
from
a4cd836 to
a5b8203
Compare
NathanDrake2406
force-pushed
the
nathan/gssp-upstream-block
branch
from
a5b8203 to
5382aff
Compare
…eneration Stale ISR background regeneration called getStaticProps directly and rendered HTML and __NEXT_DATA__ with only the page's pageProps, dropping app-level props from _app.getInitialProps. The foreground path already loads _app.getInitialProps first and merges its props, but the background regeneration path skipped that step. Extract a shared loadPagesAppInitialRenderProps helper from the production foreground path and reuse it during background regeneration in both pages-page-data.ts and dev-server.ts. This ensures regenerated pages carry the same full props envelope as the original render. Add a regression test in tests/pages-page-data.test.ts.
The foreground render path ran _app.getInitialProps before consulting the ISR cache, so fresh and stale cache hits executed userland App data code even though the cached HTML was returned verbatim. That wasted work on the hot path and ran side-effecting App code whose result the cached response never reflected. The eager App initial-props load happened up front, ahead of the HIT and STALE short-circuits. Defer it until the request will actually render: load it lazily inside the getServerSideProps / getStaticProps / getInitialProps branches, after the cache short-circuits return. Stale-while-revalidate already re-runs it inside the regeneration closure, so regenerated HTML still carries app-level props. The dev SSR handler and the production page-data resolver are updated in lockstep to preserve dev/prod parity. Tests cover the fresh-HIT skip and the stale foreground-skip vs regeneration-run split at the page-data resolver boundary.
… tested helper The dev SSR handler ran _app.getInitialProps inside an inline closure that mutated captured pageProps/renderProps and read res directly, so its lazy load-only-on-render behavior could only be covered by a heavyweight integration test that vi.doMock'd the isr-cache and file-matcher modules — a mock cascade with little signal. Move the decision into loadDevAppInitialProps in pages-get-initial-props.ts. It takes the App component, req/res, and an injected AppTree builder (keeping React out of the module) and returns a discriminated result — skip / response-sent / render — that the caller applies. The dev-server closure is now thin glue: build the React tree, delegate, apply the result. Adds focused unit tests that drive the helper through its real boundary (genuine getInitialProps functions, plain req/res inputs) — no module mocks: the skip path, App/page prop merging, pageProps normalization, the response-sent short-circuit, and the router/ctx wiring passed to userland.
|
/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: preserve app props for GSSP requests
Solid, well-tested PR. The core change — threading the full Pages props envelope (pageProps plus _app.getInitialProps app-level props and __N_SSP) through the data/render/hydration pipeline — is implemented consistently across the dev server (dev-server.ts), the prod resolver (pages-page-data.ts), and the client router shim (shims/router.ts). I verified dev/prod parity holds: both dev-server.ts and the loadPagesAppInitialRenderProps helper run _app.getInitialProps lazily (only on real renders, never on a cache HIT/STALE that serves cached HTML), and the Cloudflare/prod worker path inherits the behavior via createPagesPageHandler (which defaults originalUrl from request.url). The generatedFromDataRequest fallback-data flow is mirrored in both servers, and the new helper module (pages-get-initial-props.ts) is cleanly factored out and directly unit-tested per the repo's entry-thinning guidance.
I ran the targeted suites locally — all green:
tests/pages-page-data.test.ts(43) +tests/pages-get-initial-props.test.ts(5)tests/shims.test.ts -t "matchRewrite with external URLs"(10)tests/entry-templates.test.ts(26)tests/pages-router.test.tsGSSP + fallback-hydration casesvp checkon all changed source files (no fmt/lint/type errors)
The config-matchers.ts rewrite-param-to-query logic faithfully mirrors Next.js's prepare-destination.ts (unused source params appended to query, params consumed by path/host/hash excluded), with good test coverage including the hash-consumed case.
Comments below are minor — none are blocking.
| let element: ReactElement; | ||
| if (AppComponent) { | ||
| element = React.createElement(AppComponent, { | ||
| ...props, |
There was a problem hiding this comment.
Spreading ...props onto the App element forwards the internal envelope markers (__N_SSP, and on the gSSP-redirect path __N_REDIRECT/__N_REDIRECT_STATUS live in pageProps so those are fine, but __N_SSP/__N_SSG are top-level in props) as actual props to the user's <App>. The server-side render does the identical ...renderProps spread (dev-server.ts, pages-page-handler.ts), so client and server are self-consistent and hydration won't mismatch — but Next.js does not pass __N_SSP to <App>. It's a harmless extra prop today; worth a follow-up to strip the __N_* keys before constructing the element if you want exact parity. Not blocking.
|
|
||
| if (shouldPersistFallbackData) { | ||
| const revalidateSeconds = isrRevalidateSeconds ?? 31_536_000; | ||
| await options.isrSet( |
There was a problem hiding this comment.
This isrSet for the persisted fallback data omits tags and expireSeconds, whereas the other isrSet calls in this file pass undefined, options.expireSeconds. The generated entry will use the backend's default expiry instead of the configured expireSeconds. Likely intentional (the entry is short-lived and regenerated), but flagging for consistency:
| await options.isrSet( | |
| await options.isrSet( | |
| cacheKey, | |
| { | |
| kind: "PAGES", | |
| html: "", | |
| pageData: renderProps, | |
| generatedFromDataRequest: true, | |
| headers: undefined, | |
| status: undefined, | |
| }, | |
| revalidateSeconds, | |
| undefined, | |
| options.expireSeconds, | |
| ); |
| `, | ||
| ); | ||
| fs.writeFileSync( | ||
| path.join(rootDir, "pages", "something.jsx"), |
There was a problem hiding this comment.
The something.jsx fixture is written by writeGsspAppInitialPropsContextFixture but never requested by any test (only /blog/[post] and /rewrite-target are exercised). It looks like it was meant to cover a getServerSideProps({ query })/asPath-with-query case. Either add an assertion that fetches /something?... or drop the fixture to avoid dead test setup.
| } | ||
|
|
||
| const { route, params } = match; | ||
| req.url = originalUrl; |
There was a problem hiding this comment.
req.url = originalUrl mutates the incoming request unconditionally for every route (not just GSSP/data requests). For the common case originalUrl defaults to url so this is a no-op, but it's a side effect on shared request state late in the handler. Consider scoping the mutation to the data-fetching contexts that actually need it (gSSP req.url), or add a brief comment that this is the intended Next.js req.url contract for data-fetching methods, so a future reader doesn't mistake it for an accidental reassignment.
|
Review posted successfully to PR #1996. SummaryI reviewed PR #1996 ( What the PR does well:
Verification I ran locally (all green):
Minor findings flagged:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: preserve app props for GSSP requests
The core change — threading the full Pages props envelope (pageProps + _app.getInitialProps app-level props + __N_SSP) through data/render/hydration — is implemented thoroughly and well-tested. The config-matchers.ts rewrite-param-to-query logic faithfully mirrors Next.js's prepare-destination.ts, and the helper extraction (loadDevAppInitialProps / loadPagesAppInitialRenderProps) follows the repo's entry-thinning guidance.
I found one dev/prod parity bug worth fixing in this PR, plus a couple of minor items. AGENTS.md explicitly calls out keeping dev-server and prod-resolver request handling in sync, so I'd treat the parity item as blocking-ish rather than a follow-up.
Dev/prod parity: stale-ISR regen stores the wrong cache pageData in dev
In the prod resolver (pages-page-data.ts:872), the stale-while-revalidate background regeneration writes the full envelope: buildPagesCacheValue(freshHtml, freshRenderProps, options.statusCode). The dev server's equivalent path (dev-server.ts:1231) writes only freshPageProps. Since the regenerated HTML embeds props: freshRenderProps in __NEXT_DATA__ (dev-server.ts:1211), but the cache pageData only holds freshPageProps, a later _next/data HIT served from this regenerated entry will return just pageProps and drop the app-level props produced by _app.getInitialProps — exactly the regression this PR is fixing, but reintroduced for the dev background-regen path. There's no dev test covering a _next/data HIT after a dev stale regen. Suggested fix inline.
Verified locally (all green): tests/pages-page-data.test.ts, tests/pages-get-initial-props.test.ts, tests/shims.test.ts rewrite tests, tests/entry-templates.test.ts.
Comments below are otherwise minor.
| await isrSet( | ||
| cacheKey, | ||
| buildPagesCacheValue(freshHtml, freshProps), | ||
| buildPagesCacheValue(freshHtml, freshPageProps), |
There was a problem hiding this comment.
Dev/prod parity bug. The prod resolver writes the full props envelope here (pages-page-data.ts:872 passes freshRenderProps), but dev passes only freshPageProps. The regenerated HTML above (line 1211) already embeds props: freshRenderProps in __NEXT_DATA__, so the cache pageData is now inconsistent with the HTML — and a subsequent _next/data HIT served from this entry returns only pageProps, dropping the _app.getInitialProps app-level props this PR is meant to preserve.
| buildPagesCacheValue(freshHtml, freshPageProps), | |
| await isrSet( | |
| cacheKey, | |
| buildPagesCacheValue(freshHtml, freshRenderProps), | |
| revalidate, | |
| ); |
Worth adding a dev test that does a stale regen then a _next/data fetch and asserts app-level props survive, mirroring the prod-side coverage in pages-page-data.test.ts.
|
|
||
| if (shouldPersistFallbackData) { | ||
| const revalidateSeconds = isrRevalidateSeconds ?? 31_536_000; | ||
| await options.isrSet( |
There was a problem hiding this comment.
The persisted fallback-data isrSet omits expireSeconds, whereas the regen isrSet above (line 870) passes undefined, options.expireSeconds. This entry will fall back to the backend default expiry instead of the configured one. Likely intentional (short-lived, regenerated on next miss), but for consistency:
| await options.isrSet( | |
| await options.isrSet( | |
| cacheKey, | |
| { | |
| kind: "PAGES", | |
| html: "", | |
| pageData: renderProps, | |
| generatedFromDataRequest: true, | |
| headers: undefined, | |
| status: undefined, | |
| }, | |
| revalidateSeconds, | |
| undefined, | |
| options.expireSeconds, | |
| ); |
| `, | ||
| ); | ||
| fs.writeFileSync( | ||
| path.join(rootDir, "pages", "something.jsx"), |
There was a problem hiding this comment.
The something.jsx fixture is written by writeGsspAppInitialPropsContextFixture but never fetched by any test (only /blog/[post], /blog-*, and /rewrite-source/* are exercised). It looks intended to cover a getServerSideProps({ query }) / asPath-with-query case. Either add an assertion that fetches /something?... or drop the fixture to avoid dead test setup.
| } | ||
|
|
||
| const { route, params } = match; | ||
| req.url = originalUrl; |
There was a problem hiding this comment.
req.url = originalUrl mutates the incoming request unconditionally for every route, not just GSSP/data requests. For the common case originalUrl defaults to url, so it's a no-op, but it's a side effect on shared request state. Consider scoping the assignment to the data-fetching contexts that actually consume req.url, or add a brief comment noting this implements the Next.js req.url contract for data-fetching methods so a future reader doesn't read it as an accidental reassignment.
| let element: ReactElement; | ||
| if (AppComponent) { | ||
| element = React.createElement(AppComponent, { | ||
| ...props, |
There was a problem hiding this comment.
Spreading ...props onto the App element forwards top-level envelope markers like __N_SSP/__N_SSG as actual props to the user's <App>. The server render does the identical ...renderProps spread (dev-server.ts, pages-page-handler.ts), so client/server are self-consistent and hydration won't mismatch — but Next.js does not pass __N_SSP to <App>. Harmless today; worth a follow-up to strip __N_* keys before constructing the element for exact parity. Not blocking.
|
Review posted successfully to PR #1996. SummaryI reviewed PR #1996 ( Main finding (dev/prod parity bug — worth fixing in this PR): The stale-while-revalidate background regeneration in the dev server ( Minor items flagged:
Verification I ran locally: confirmed the dev/prod divergence by reading both The first review submission hit a 422 because I'd put a wrong line number on the |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: preserve app props for GSSP requests
Thorough, well-tested change. The full Pages props envelope (pageProps + _app.getInitialProps app-level props + __N_SSP) is threaded consistently through data/render/hydration across the dev server, the prod resolver, the prod page handler, and the client router shim. I verified the key correctness properties:
- Dev/prod parity on app-GIP timing holds. Both
loadDevAppInitialPropsandloadPagesAppInitialRenderPropsrun_app.getInitialPropslazily — only on real renders, never on a fresh/stale ISR HIT that serves cached HTML (the HIT short-circuits inpages-page-data.tsare gated on!isDataReq, so_next/datarequests always regenerate the full envelope rather than reading stalepageData). The Cloudflare worker path inherits the behavior throughcreatePagesPageHandler'soriginalUrldefaulting. - The two parity items prior reviews flagged are already fixed in the current head. The dev stale-regen now writes
buildPagesCacheValue(freshHtml, freshRenderProps)(dev-server.ts:1239), matching prod (pages-page-data.ts:872), and the prod fallback-persistisrSetnow passesoptions.expireSeconds(pages-page-data.ts:985). config-matchers.tsrewrite-param-to-query faithfully mirrors Next.jsprepare-destination.ts(unused source params appended to query; params consumed by path/host/hash excluded). The new/[^/]:[\w-]+/regex-mode trigger is scoped to source path patterns, so schemes likehttps://are not a concern.- The
_h: 1query-updating navigation (fallbackRouter.replace) correctly suppressesrouteChangeStart/routeChangeCompleteand bypasses the hash-only-change early return so the data fetch still happens, matching Next.js's query-update mechanism.
Verified locally (all green): tests/pages-page-data.test.ts (43) + tests/pages-get-initial-props.test.ts (5), and tests/shims.test.ts rewrite tests (28).
One minor cleanup item below; nothing blocking.
| `, | ||
| ); | ||
| fs.writeFileSync( | ||
| path.join(rootDir, "pages", "something.jsx"), |
There was a problem hiding this comment.
The something.jsx fixture is written here but never requested by any test. The GSSP context test (line 1517) only exercises /blog/[post], /blog-post-*, and /rewrite-source/* — /something is never fetched, so its getServerSideProps({ query }) / asPath-with-query assertions never run. Either add a fetch + assertions for /something?... (it looks intended to cover the query-merge / initial-query case) or drop the fixture to avoid dead test setup. This was flagged on two prior review passes and still persists.
| } | ||
|
|
||
| const { route, params } = match; | ||
| req.url = originalUrl; |
There was a problem hiding this comment.
req.url = originalUrl mutates the incoming request unconditionally for every matched route, not just GSSP/data requests. For the common case originalUrl defaults to url, so it is a no-op, but it is a side effect on shared request state read later at line 1090 (regenReq = { url: req.url, ... }). It is in fact intentional — it implements the Next.js req.url contract for data-fetching methods (the data test asserts appProps.url === "/_next/data/.../blog/post-1.json?hello=world"). Worth a one-line comment noting that, so a future reader does not mistake it for an accidental reassignment.
|
Review posted successfully to PR #1996. SummaryI reviewed PR #1996 ( What I verified (and where this review differs from prior passes): Two of the items that earlier review passes flagged as bugs have since been fixed in the current head — I confirmed this by reading the actual source rather than relying on the prior diffs:
I also independently chased down a potential new bug — the initial ISR write storing only Verified locally (all green): Remaining minor items (nothing blocking):
The core implementation is sound, dev/prod parity holds, and the |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: preserve app props for GSSP requests
Thorough, well-tested change. I independently verified the key correctness properties rather than relying on the prior review passes:
- Dev/prod parity holds across all four request paths. Both the dev server (via
pipelineDeps→runPagesRequest) and the prod/worker paths sharepages-request-pipeline.ts, so the newx-nextjs-rewriteheader on data responses (pages-request-pipeline.ts:368) is emitted identically in dev, prod, and Workers — the client-navigation "hydrate middleware rewrite to fallback page" feature works the same everywhere. The dev server has no separatex-nextjs-rewritecodepath to drift out of sync. - App-GIP timing is lazy in both dev and prod. The fresh/stale ISR HIT short-circuits in
pages-page-data.tsare gated on!isDataReqand thegeneratedFromDataRequestforeground branch skipsloadForegroundAppInitialRenderProps()entirely, so_app.getInitialPropsnever runs on a cache hit. The dedicated tests (does not run _app.getInitialProps on a fresh ISR cache HIT,only runs ... in the stale ISR regeneration path) lock this in. - The two parity items earlier passes flagged are confirmed fixed in the current head: the dev stale-regen and prod stale-regen both write the full envelope via
buildPagesCacheValue(freshHtml, freshRenderProps), and the foreground fallback-persistisrSetnow passesoptions.expireSeconds(pages-page-data.ts:985). config-matchers.tsrewrite-param-to-query faithfully mirrors Next.jsprepare-destination.ts: unused source params are appended to query, params consumed by the destination path/host/hash are excluded, andnextInternalLocaleis filtered. ThegetDestinationPathAndHostscheme-stripping correctly avoids treatinghttps://as a path-consumed param.- The redirect data envelope now threads
__N_SSPand app-level props as siblings ofpagePropswhile keeping__N_REDIRECT/__N_REDIRECT_STATUSinsidepageProps, matching Next.js'spageProps.__N_REDIRECTcontract.
Two small cleanup items below; nothing blocking. The __N_SSG flag for gSP pages is still never set (Next.js sets it) — that and the __N_*-leaking-to-<App> parity item from prior passes are reasonable follow-ups, not blockers, since client and server are self-consistent.
| `, | ||
| ); | ||
| fs.writeFileSync( | ||
| path.join(rootDir, "pages", "something.jsx"), |
There was a problem hiding this comment.
The something.jsx fixture is written here but never requested by any test. The GSSP context test (passes original req.url, query, asPath, and resolvedUrl through _app.getInitialProps) only fetches /blog/post-1, the /blog-post-* rewrites, and /rewrite-source/foo — /something is never fetched, so its getServerSideProps({ query }) / initial-query / asPath-with-query assertions never run. Either add a fetch + assertions for /something?... (it looks intended to cover the query-merge case) or drop the fixture to avoid dead test setup. This has persisted across all three prior review passes.
| } | ||
|
|
||
| const { route, params } = match; | ||
| req.url = originalUrl; |
There was a problem hiding this comment.
req.url = originalUrl mutates the incoming request unconditionally for every matched route, not just GSSP/data requests. For the common case originalUrl defaults to url so it's a no-op, but it is a side effect on shared request state that is read again later (e.g. the regen request snapshot at regenReq = { url: req.url, ... }). It is intentional — it implements the Next.js req.url contract for data-fetching methods (the data test asserts appProps.url === "/_next/data/.../blog/post-1.json?hello=world"). Worth a one-line comment so a future reader doesn't mistake it for an accidental reassignment.
| req.url = originalUrl; | |
| // Implements the Next.js `req.url` contract: data-fetching methods | |
| // (gSSP/gSP `_app.getInitialProps`) observe the original request URL. | |
| // Defaults to `url` for non-data requests, so this is a no-op there. | |
| req.url = originalUrl; |
|
Review posted successfully to PR #1996. SummaryI reviewed PR #1996 ( What I independently verified (rather than trusting prior passes):
Minor items flagged (non-blocking):
The core implementation is sound and well-tested. |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
This reverts commit cbdcdf6.
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: preserve app props for GSSP requests
Thorough, well-tested change. The full Pages props envelope (pageProps + _app.getInitialProps app-level props + __N_SSP) is threaded consistently across the dev server, prod resolver, prod page handler, and client router shim. I independently verified the key correctness properties this pass:
- The
beforeFileschained-rewrite change is correct and matches Next.js. The PR changespages-request-pipeline.tsStep 9 from a single first-match rewrite to a sequential loop that re-feeds each rewritten pathname into the nextbeforeFilesrule. I confirmed against Next.jsresolve-routes.ts: when a non-externalbeforeFilesrewrite matches, Next setsdidRewrite = true, updatesparsedUrl.pathname, and does not return — it continues the route loop so the nextbeforeFilesrule matches against the already-rewritten path. The new test ("applies every matching beforeFiles rewrite in sequence") encodes correct behavior. config-matchers.tsrewrite-param-to-query faithfully mirrors Next.jsprepare-destination.ts— unused source params appended to query, params consumed by destination path/host/hash excluded,nextInternalLocalefiltered, scheme-stripping ingetDestinationPathAndHostpreventshttps://being misread as a path-consumed param.- App-GIP runs lazily — fresh/stale ISR HIT short-circuits are gated on
!isDataReqand thegeneratedFromDataRequestforeground branch skips app GIP, locked in by dedicated tests. Dev stale-regen and prod stale-regen both write the full envelope. The two parity bugs from earlier passes are confirmed fixed. mergeRewriteQuerycorrectly preserves the original query (incl. RSC_rsc) on App Router rewrites, so theurl.search = ""branch inapplyInternalRewriteDestinationonly fires when both original and rewrite are query-less.
Verified locally (all green): pages-page-data.test.ts (43), pages-get-initial-props.test.ts (5), pages-request-pipeline.test.ts (44), entry-templates.test.ts (26), shims.test.ts rewrite tests (28), app-router-next-config-dev.test.ts (16), plus vp check on the changed source files.
One new gap worth addressing plus two persistent minor items below. Nothing blocking.
| ); | ||
| if (beforeFilesRewrite instanceof Response) return beforeFilesRewrite; | ||
| if (beforeFilesRewrite) cleanPathname = beforeFilesRewrite; | ||
| if (beforeFilesRewrite) cleanPathname = applyInternalRewriteDestination(beforeFilesRewrite, url); |
There was a problem hiding this comment.
New App Router behavior with no App Router test. This PR routes the App Router's beforeFiles/afterFiles/fallback rewrites through the new applyInternalRewriteDestination, which (via config-matchers.ts) appends unused source params to the destination query and merges them into url.search. That's a real cross-cutting change to App Router request handling — e.g. a config rewrite { source: "/blog-:param", destination: "/blog/post-3" } now exposes ?param=... to the App Router page's searchParams, and url.search mutation feeds downstream searchParams.
The new tests cover this for the Pages Router (pages-request-pipeline.test.ts, pages-router.test.ts) and the matcher itself (shims.test.ts), but there is no App Router test asserting that a rewrite's unused source params reach searchParams (or that the original query/_rsc survives). app-router-next-config-dev.test.ts already has a rewrite fixture harness — please add a case there so this App Router code path is covered and protected against regression.
| `, | ||
| ); | ||
| fs.writeFileSync( | ||
| path.join(rootDir, "pages", "something.jsx"), |
There was a problem hiding this comment.
The something.jsx fixture is written here but never requested by any test — the GSSP context test at line 1517 only fetches /blog/post-1, /blog-post-2, /blog-post-3, and /rewrite-source/foo. Its getServerSideProps({ query }) / initial-query / asPath-with-query assertions never run. Either add a /something?... fetch + assertions (it looks intended to cover the query-merge case) or drop the fixture. This has persisted across all prior review passes.
| } | ||
|
|
||
| const { route, params } = match; | ||
| req.url = originalUrl; |
There was a problem hiding this comment.
req.url = originalUrl mutates the incoming request unconditionally for every matched route, not just GSSP/data requests. For the common case originalUrl defaults to url so it's a no-op, but it is a side effect on shared request state read again later (the regen snapshot regenReq = { url: req.url, ... }). It is intentional — it implements the Next.js req.url contract for data-fetching methods (the data test asserts appProps.url === "/_next/data/.../blog/post-1.json?hello=world"). Worth a one-line comment so a future reader doesn't mistake it for an accidental reassignment.
| req.url = originalUrl; | |
| // Implements the Next.js `req.url` contract: data-fetching methods | |
| // (gSSP/gSP `_app.getInitialProps`) observe the original request URL. | |
| // Defaults to `url` for non-data requests, so this is a no-op there. | |
| req.url = originalUrl; |
|
Review posted successfully to PR #1996. SummaryI reviewed PR #1996 ( What I independently verified this pass (focusing on things prior passes hadn't):
Verified locally (all green): Minor persistent items flagged: the unused Nothing blocking. |
|
/bigbonk review for issues |
5 similar comments
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: preserve app props for GSSP requests
Thorough, well-tested change. The full Pages props envelope (pageProps + _app.getInitialProps app-level props + __N_SSP) is threaded consistently across the dev server, prod resolver, prod page handler, and client router shim. The two parity bugs earlier passes flagged are confirmed fixed in the current head (dev + prod stale-regen both write the full envelope; the foreground fallback-persist isrSet passes options.expireSeconds), and config-matchers.ts rewrite-param-to-query faithfully mirrors Next.js prepare-destination.ts.
Verified locally (all green): pages-page-data (43), pages-get-initial-props (5), pages-request-pipeline (44).
One new internal-consistency item and one coverage gap below, plus the two persistent minor items. Nothing blocking.
Foreground ISR write stores page-only pageData while the regen path stores the full envelope
The foreground ISR cache writes store pageData: pageProps (page-only) in both dev (dev-server.ts:1786, buildPagesCacheValue(isrHtml, pageProps)) and prod (pages-page-response.ts:616, pageData: options.pageProps), while the stale-regen paths now store the full freshRenderProps envelope (dev-server.ts:1239, pages-page-data.ts:869) — and the cached HTML embeds the full renderProps in __NEXT_DATA__ in all cases. So the pageData field is inconsistent between the foreground write (page-only) and the regen write (full envelope) for the same entry kind.
I confirmed this is not currently live-observable for non-fallback gSP pages: the fresh/stale cache-HIT short-circuits are gated on !isDataReq, so _next/data requests always bypass the HIT and regenerate the full envelope, and the only path that reads pageData back as the data response (generatedFromDataRequest entries) correctly stores renderProps. So no app-level props are dropped today. But the foreground vs regen pageData divergence is fragile — a future change that reads pageData back on a non-regen entry (e.g. an ISR data HIT optimization) would silently drop app-level props. Worth either aligning both foreground writes to store renderProps, or a one-line comment on each foreground write explaining why page-only pageData is intentional there. (Both foreground-write lines are unchanged context, so I couldn't attach inline comments there.)
App Router rewrite-param-to-query change has no App Router test
The PR routes App Router beforeFiles/afterFiles/fallback rewrites through the new applyInternalRewriteDestination, which appends unused rewrite source params to the destination query and mutates url.search (feeding App Router searchParams). mergeRewriteQuery correctly preserves the original query/_rsc, so it's behaviorally sound, but coverage exists only for Pages Router (pages-request-pipeline.test.ts, pages-router.test.ts) and the matcher (shims.test.ts). app-router-next-config-dev.test.ts has no case asserting that a rewrite's unused source params reach searchParams (or that _rsc survives). Please add one to that existing harness so this cross-cutting App Router code path is protected against regression.
| ); | ||
| if (beforeFilesRewrite instanceof Response) return beforeFilesRewrite; | ||
| if (beforeFilesRewrite) cleanPathname = beforeFilesRewrite; | ||
| if (beforeFilesRewrite) cleanPathname = applyInternalRewriteDestination(beforeFilesRewrite, url); |
There was a problem hiding this comment.
New App Router behavior with no App Router test: routing beforeFiles/afterFiles/fallback rewrites through applyInternalRewriteDestination now appends unused rewrite source params to the destination query and mutates url.search, which feeds App Router searchParams. The behavior is sound (mergeRewriteQuery preserves the original query/_rsc), but only Pages Router + the matcher are tested. Please add a case to app-router-next-config-dev.test.ts asserting a config rewrite like { source: "/blog-:param", destination: "/blog/post-3" } exposes ?param=... to the App Router page's searchParams and that _rsc survives.
| `, | ||
| ); | ||
| fs.writeFileSync( | ||
| path.join(rootDir, "pages", "something.jsx"), |
There was a problem hiding this comment.
The something.jsx fixture is written here but never requested — the GSSP context test (line 1517) only fetches /blog/post-1, /blog-post-2, /blog-post-3, and /rewrite-source/foo. Its getServerSideProps({ query }) / asPath-with-query assertions never run. Either add a /something?... fetch + assertions (it looks intended to cover the query-merge case) or drop the fixture. This has persisted across all prior review passes.
| } | ||
|
|
||
| const { route, params } = match; | ||
| req.url = originalUrl; |
There was a problem hiding this comment.
req.url = originalUrl mutates the incoming request unconditionally for every matched route, not just GSSP/data requests. For the common case originalUrl defaults to url so it's a no-op, but it is a side effect on shared request state read again later (the regen snapshot regenReq = { url: req.url, ... }). It is intentional — it implements the Next.js req.url contract for data-fetching methods (the data test asserts appProps.url === "/_next/data/.../blog/post-1.json?hello=world"). A one-line comment would prevent a future reader from mistaking it for an accidental reassignment.
| req.url = originalUrl; | |
| // Implements the Next.js `req.url` contract: data-fetching methods | |
| // (gSSP/gSP `_app.getInitialProps`) observe the original request URL. | |
| // Defaults to `url` for non-data requests, so this is a no-op there. | |
| req.url = originalUrl; |
|
Review posted successfully to PR #1996. SummaryI reviewed PR #1996 ( What I independently verified (reading source, not trusting prior passes):
Findings flagged:
Note: two prior inline comment targets ( |
|
/bigbonk review for issues |
1 similar comment
|
/bigbonk review for issues |
2 participants
Summary
Ports the scoped Pages Router getServerSideProps behavior from upstream Next.js and fixes vinext's Pages render/data pipeline to preserve full
_app.getInitialPropsprops for GSSP/GSP requests.This PR makes Pages Router handling match the upstream contract for:
_next/dataJSON returning the full props object (pagePropsplus app-level props) when_app.getInitialPropsis present_app.getInitialProps(ctx)receiving the originalreq.url, routepathname, mergedquery, and visibleasPathgetServerSideProps({ resolvedUrl })using the resolved page pathname plus the original request search/blog-:param -> /blog/post-3carrying unused source params into the target queryUpstream references
_app.getInitialPropsfixture: https://github.com/vercel/next.js/blob/canary/test/e2e/getserversideprops/app/pages/_app.js_next/datanormalization in base server: https://github.com/vercel/next.js/blob/canary/packages/next/src/server/base-server.tsgetServerSideProps: https://nextjs.org/docs/pages/api-reference/functions/get-server-side-propsVerification
vp test run --project integration tests/pages-router.test.tsvp test run tests/shims.test.ts -t "matchRewrite with external URLs"vp checkvp env exec --node 24 ./scripts/run-nextjs-deploy-suite.sh /Users/nathan/Projects/vinext/.refs/nextjs-v16.2.6 --retries 0 -c 1 --debug test/e2e/getserversideprops/test/index.test.ts#__NEXT_DATA__script parsing, server data request dedupe, and invalid JSON GSSP fallback behavior.