fix(pages): align React runtime behavior#2571
Conversation
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 |
|
/bigbonk review for issues |
|
/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 — PR #2571 (fix(pages): align React runtime behavior)
Reviewed the full branch (2812400 + b18676c) against main. Ran tests/dynamic.test.ts, tests/pages-page-data.test.ts, tests/pages-page-response.test.ts, tests/seed-cache.test.ts, tests/entry-templates.test.ts, tests/pages-router.test.ts (all green) and tsc --noEmit (clean).
The overall approach is sound: request-scoped dynamicIds recording, imperative route announcer that produces no SSR markup (so no hydration mismatch), and prerender seeding to avoid the double auto-export render. Below are the issues worth addressing.
1. App-load-failure fallback path skips dynamic preload (parity gap)
packages/vinext/src/entries/pages-client-entry.ts (~L253-267):
let appModule;
try {
appModule = await appLoader();
await preloadPagesDynamicModules(nextData.dynamicIds);
...
} catch {
element = React.createElement(PageComponent, pageProps); // no preload here
}If appLoader() throws, the catch hydrates PageComponent without calling preloadPagesDynamicModules(...). That reintroduces the exact hydration-timing bug this PR fixes, but only on the _app load-failure fallback. preloadPagesDynamicModules swallows loader errors internally, so it's safe to call it before building the fallback element. Suggest moving the preload out of the try (or calling it in the catch) so both branches preload.
2. Dev/prod parity: dev client template has no _app failure fallback
packages/vinext/src/server/dev-server.ts (~L1820-1834) generates the dev hydration script with a bare const appModule = await import(...) and no try/catch, whereas the prod template (pages-client-entry.ts) wraps _app loading in try/catch with a PageComponent fallback. This is a pre-existing divergence, but issue #1's fix should be mirrored to whichever template ends up with the fallback so the two stay in sync per AGENTS.md's dev/prod parity rule.
3. preloadPagesDynamicModules clears all registered initializers, not just requested ones
packages/vinext/src/shims/pages-dynamic.ts (L50-51): clientInitializers.clear() wipes the entire map each loop iteration, but only initializers whose id is in requestedIds are actually invoked (L54). Any initializer registered for a module not in nextData.dynamicIds is silently discarded, and since clientPreloadFinished is then latched true (L61), registerPagesDynamicInitializer becomes a permanent no-op (L31). In practice this is fine because post-hydration components load lazily via React.lazy/Suspense, but the "clear everything, run a subset" shape is a footgun. Consider deleting only the entries you actually ran, and add a comment documenting that the latch is intentional (initial-hydration-only preload).
4. Dead defensive branch in preloadPagesDynamicModules
The inner IIFE (L48-62) can never reject — every pending promise is .catch(() => undefined)'d — so clientPreloadFinished is always true by the time the finally (L66-69) runs, making the clientPreloadPromise = undefined reset unreachable. Not a bug, but the try/finally implies a rejection path that doesn't exist; either drop it or make the intent explicit.
Non-blocking notes
- Shared module-level retry state (
sharedSnapshot/retrySharedSnapshotindynamic.ts) means all instances of the samedynamic()share one retry/loadable state, and a successful load persists across unmount/remount. The newpreserves a successful dynamic retry across remountstest asserts exactly this (gateRequests === 2). This matchesreact-loadable's shared-subscription model, so it's consistent with Next.js — just calling it out as intentional since it changes prior per-instanceuseStatebehavior. - Route announcer text priority is
document.title || h1 || asPath, evaluated in auseEffectonasPathchange — correct ordering and it reads the post-navigation title. The initial-render guard (previousPath.current === asPath) correctly suppresses the initial announcement, satisfying the "should not have the initial route announced" case. - Seed-cache: seeding reads full prerendered HTML from disk (which already contains
__NEXT_DATA__incl.dynamicIds), and the auto-export HIT inpages-page-data.tsserves it verbatim, sodynamicIdsare preserved on the seeded path. Key derivation (routeUrl.split("?")[0]) dropping the query is correct for auto-export/static pages.
Items 1–2 are the ones I'd want addressed before merge (hydration correctness + dev/prod parity); 3–4 are cleanup.
|
Review posted to PR #2571. Summary of findings: Review SummaryI reviewed the full branch (both Blocking-ish (hydration correctness + parity):
Cleanup: Non-blocking notes: confirmed the shared module-level retry state matches |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review — PR #2571 (fix(pages): align React runtime behavior)
Re-reviewed the full branch at 9f183c7 (includes the follow-up d1e64d2 fix(pages): preserve preload on app fallback) against main. Ran tests/dynamic.test.ts, tests/pages-page-data.test.ts, tests/pages-page-response.test.ts, tests/seed-cache.test.ts, tests/entry-templates.test.ts (196/196 pass) and vp check on the changed source files (clean).
Status of prior review items
- #1 (app-load-failure skips dynamic preload): ✅ Fixed.
preloadPagesDynamicModules(nextData.dynamicIds)now runs after thetry/catchinpages-client-entry.ts:272, so both the success and_app-failure fallback branches preload before hydration. - #2 (dev/prod parity): ✅ Fixed. The dev hydration template in
dev-server.tsnow wraps_apploading intry/catchwith aPageComponentfallback and callspreloadPagesDynamicModules(nextData.dynamicIds), matching the prod template. - #3 (clear-all initializers footgun):
⚠️ Still present (see below). - #4 (dead defensive branch): ✅ Fixed. The
try/finallywas removed;clientPreloadFinished = truenow runs inline in the IIFE.
New / remaining findings
1. Cloudflare runtime parity gap — auto-export seed never fires on Workers (parity, non-blocking for stated scope)
The double-render fix has two halves: (a) seed prerendered Pages HTML into the cache at startup, and (b) short-circuit auto-export pages on a cache HIT in pages-page-data.ts:745-763. The seed half only runs in the Node production server — startPagesRouterServer calls seedMemoryCacheFromPrerender(...) at prod-server.ts:1634.
On Cloudflare Workers there is no equivalent:
packages/cloudflare/src/prerender-kv-populate.ts(buildPrerenderKVPairs) only walksgetRenderedAppRoutes(...)and writeskind: "APP_PAGE"entries — it never seeds Pages Router (kind: "PAGES") artifacts.- The default
MemoryCacheHandleris empty on isolate cold start, and the auto-export path inpages-page-data.tsonly reads the cache (unlike thegetStaticPropspath, it neverisrSets after render).
Net effect: on Workers the auto-export HIT never fires, so the behavior the upstream should only render once in SSR test verifies won't hold there. This is consistent with AGENTS.md's "MemoryCacheHandler is empty per-isolate" note, so it's arguably out of scope for a PR targeting the Node-based upstream e2e suite — but it's a deliberate divergence worth either documenting in the PR or filing as a follow-up (mirror the Pages seed into buildPrerenderKVPairs).
2. Route announcer uses textContent where Next.js uses innerText first (minor parity)
shims/router.ts PagesRouteAnnouncer computes document.title || heading?.textContent || asPath. Next.js's route announcer prefers h1?.innerText before falling back to textContent (document.title || innerText || textContent || pathname). textContent includes visually-hidden text and doesn't collapse whitespace, so for pages that rely on the <h1> (no document.title) the announced string can differ from Next.js. Suggest heading?.innerText || heading?.textContent to match.
3. Auto-export cache HIT omits bot ETag / 304 handling (consistency)
The new auto-export HIT at pages-page-data.ts:749-762 returns buildPagesCacheResponse(..., "HIT", ...) directly, but the analogous getStaticProps HIT path (pages-page-data.ts:886-904) additionally runs applyBotETagAndCheck(...) so bot UAs get a stable ETag and can receive a 304 on If-None-Match. Auto-export HITs skip that, so a crawler hitting an auto-export page gets no ETag/304 while a crawler hitting an SSG page does. Not a correctness bug for the target failures, but an inconsistency between two very similar HIT branches.
4. preloadPagesDynamicModules still clears all registered initializers (from prior #3)
shims/pages-dynamic.ts:50-56: each loop iteration snapshots clientInitializers, then clientInitializers.clear() wipes the whole set, but only initializers whose registered ids intersect requestedIds actually run (registerPagesDynamicInitializer returns undefined otherwise). Any initializer for a module not in nextData.dynamicIds is discarded, and once clientPreloadFinished = true latches, registerPagesDynamicInitializer is a permanent no-op. This is fine in practice (post-hydration components load lazily via React.lazy/Suspense) but the "clear everything, run a subset" shape remains a footgun. Consider deleting only the entries you ran and adding a one-line comment that the latch is intentionally initial-hydration-only.
Non-blocking confirmations
- Shared module-level dynamic retry state (
sharedSnapshot/retrySharedSnapshot+useSyncExternalStoreindynamic.ts) intentionally shares one loadable/retry state across instances and persists a successful load across remount — matchingreact-loadable. Thepreserves a successful dynamic retry across remountstest (gateRequests === 2) asserts exactly this. The addedkey: retryKeyon the error boundary correctly forces a remount on retry. - Seed-cache path reads full prerendered HTML from disk (which already embeds
__NEXT_DATA__incl.dynamicIds) and serves it verbatim on the auto-export HIT, sodynamicIdsare preserved. Cache-key derivation (routeUrl.split("?")[0]+ buildId via the request-scopedisrCacheKey) matches the seeder'sisrCacheKey("pages", cachePathname, buildId). dynamicIdsserialization into__NEXT_DATA__is consistently gated ongetPagesDynamicModuleIds()returning a non-empty array in bothpages-page-response.tsand the devbuildAllScripts()closure — no empty-array noise.
Items 1–2 are the ones I'd want addressed or explicitly deferred (Cloudflare parity + announcer text parity); 3–4 are consistency/cleanup.
|
Review posted to PR #2571. SummaryRe-reviewed the full branch at Prior review items resolved:
New findings flagged:
Plus non-blocking confirmations that the shared retry state, seed-cache The overall approach is sound and the two blocking items from the prior review are properly fixed. Items 1–2 are worth addressing or explicitly deferring; 3–4 are cleanup. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review — PR #2571 (fix(pages): align React runtime behavior)
Re-reviewed the full branch at 1c6eb40 (includes the new follow-up 1c6eb40 fix(pages): preserve prerender runtime parity, on top of the previously reviewed 9f183c7) against main. Ran tests/pages-page-data.test.ts, tests/seed-cache.test.ts, tests/prerender-kv-populate.test.ts (97/97), tests/dynamic.test.ts (19/19), and tests/entry-templates.test.ts + tests/pages-router.test.ts (393/393). Ran vp check on the changed source files (clean).
Status of prior review items — all three flagged items are now fixed
- Prior #1 (Cloudflare runtime parity — auto-export seed never fires on Workers): ✅ Fixed.
buildPrerenderKVPairsnow has a second loop (prerender-kv-populate.ts:176-219) that walksroute.router === "pages"rendered routes, skips fallback shells and zero-revalidate entries, and seedskind: "PAGES"KV entries underisrCacheKey("pages", cachePathname, buildId)— the same key the auto-export HIT reads. Error-doc status (/404→404,/500//_error→500) is preserved, matching the Node seed path. Covered by newtests/prerender-kv-populate.test.tscases (pages seeding, fallback/zero-revalidate skipping, error-doc status). - Prior #2 (route announcer
textContentvs Next.jsinnerText): ✅ Fixed.PagesRouteAnnouncernow usesheading?.innerText ?? heading?.textContent(router.ts:1437), exactly matching Next.js'sroute-announcer.tsx(pageHeader?.innerText ?? pageHeader?.textContent). Verified against upstream canary source. New Playwright testroute announcer uses visible heading text when the title is emptycovers the<h1>fallback path. - Prior #3 (auto-export HIT omits bot ETag / 304 handling): ✅ Fixed. The auto-export HIT at
pages-page-data.ts:749-764now callsapplyBotETagAndCheck(hitResponse, cachedValue.html, options)before returning, identical to thegetStaticPropsHIT path (pages-page-data.ts:902). Crawlers now get a stable ETag / 304 on both HIT branches.
Prior blocking items (from the review before) — confirmed still fixed
- App-load-failure skips dynamic preload: ✅
preloadPagesDynamicModules(nextData.dynamicIds)runs after thetry/catchin both prod (pages-client-entry.ts:272) and dev (dev-server.ts) hydration templates. - Dev/prod parity: ✅ dev template mirrors prod (try/catch
_appload +PageComponentfallback + preload). - Dead defensive branch: ✅
try/finallyremoved frompreloadPagesDynamicModules.
Remaining findings
1. preloadPagesDynamicModules still clears all registered initializers (cleanup, non-blocking; carried over from prior #3/#4)
shims/pages-dynamic.ts:50-51: each loop iteration snapshots clientInitializers then clientInitializers.clear() wipes the whole set, but only initializers whose ids intersect requestedIds actually run (registerPagesDynamicInitializer returns undefined otherwise). Initializers for modules not in nextData.dynamicIds are discarded, and once clientPreloadFinished = true latches, registerPagesDynamicInitializer becomes a permanent no-op. This is fine in practice (post-hydration components load lazily via React.lazy/Suspense) but the "clear everything, run a subset" shape remains a footgun. Consider deleting only the entries you ran and adding a one-line comment that the latch is intentionally initial-hydration-only.
2. Auto-export HIT does not bypass on-demand revalidation (minor, likely intended)
The auto-export HIT guard (pages-page-data.ts:745) is isAutoExport && !options.isDataReq && !options.scriptNonce && previewData === false, and since previewData is forced to false when isOnDemandRevalidate is true (pages-page-data.ts:722), an on-demand res.revalidate() request against an auto-export page still serves the cached HIT rather than falling through (unlike the getStaticProps HIT, which explicitly checks !options.isOnDemandRevalidate). Because auto-export pages have no data-fetching to regenerate, serving the cached HTML is arguably correct — flagging only so the divergence is a conscious choice, not accidental.
Non-blocking confirmations
dynamicIdsrender/serialize ordering is correct in all three paths: in prod (pages-page-response.ts:560) and dev (buildAllScripts()atdev-server.ts) the module ids are read afterrenderToReadableStreamresolves the shell —ServerDynamicrecordspreloadModuleIdssynchronously during shell render, so the ids are populated beforegetPagesDynamicModuleIds()runs. The devscriptscallback is invoked atdev-server.ts:458/468, afterrenderToReadableStreamat line 407. Serialization is gated on a non-empty array in both paths (no empty-array noise). Verified by the newserializes only next/dynamic modules used during the request renderunit test.- Node/Cloudflare seed parity for revalidate: static pages (
revalidate: false) resolve to31_536_000on both paths (Cloudflare inlines the default atprerender-kv-populate.ts:193; Node'sisrSetPrerenderedPagesPagedefaultsrevalidateSeconds ?? 31_536_000atisr-cache.ts:242), socacheControl.revalidatematches. Error-doc status derivation is identical betweenseed-cache.ts:172-177andprerender-kv-populate.ts:204-209. - Shared module-level dynamic retry state (
sharedSnapshot/retrySharedSnapshot+useSyncExternalStore) intentionally shares one loadable/retry state across instances and persists a successful load across remount — matchingreact-loadable. On the server this correctly caches the module load once per isolate; retry only fires from client error boundaries. The addedkey: retryKeyon the error boundary forces a remount on retry. - Route announcer matches Next.js's title→h1→asPath priority and suppresses the initial announcement via
previousPath.current === asPath.
Verdict: The three items I flagged in the previous review are all properly resolved and covered by tests, and the two earlier blocking items remain fixed. Remaining findings (#1 cleanup, #2 minor divergence) are non-blocking. The approach is sound and the branch looks ready to merge.
Reviewed at 1c6eb40.
|
Review posted to PR #2571. SummaryI re-reviewed the full branch at the latest commit The key outcome: all three items flagged in the prior review are now fixed and tested:
The earlier blocking items (app-load-failure preload, dev/prod parity, dead defensive branch) also remain fixed. Remaining (non-blocking):
Verdict: sound approach, prior findings resolved and covered by tests, ready to merge. |
Independent Codex review — NO FINDINGSIndependently reviewed exact head Scope reviewed:
Targeted oracle evidence:
Verdict: NO FINDINGS. No blocking correctness, parity, or regression concerns in the reviewed scope. |
1 participant
Summary
Fix the Pages Router functional failures from
test/e2e/react-current-version/react-current-version.test.tsin Actions run28938793088against Next.jsv16.3.0-canary.80.Owned non-styled failures:
Basics > should only render once in SSR(expected 1 render, received 2)Basics > should contain dynamicIds in next data for dynamic importsConcurrent mode in the experimental-edge runtime > <RouteAnnouncer /> > should not have the initial route announcedConcurrent mode in the nodejs runtime > <RouteAnnouncer /> > should not have the initial route announcedThis change:
next/dynamicmodule usage and serializes rendered module IDs in__NEXT_DATA__.dynamicIdsScope
The two
flushes styled-jsx styles as the page rendersassertions remain intentionally excluded because styled-jsx/CSS behavior is owned by #2220. This PR does not modify styled-jsx or CSS handling.No overlapping open PR from
james-elicxwas found for these functional failures.Validation
Pinned upstream suite:
Result: 9/9 non-styled functional assertions pass. The command exits nonzero only for the 2/2 explicitly excluded styled-jsx assertions.
Focused vinext tests:
vp test run tests/pages-page-data.test.ts tests/pages-page-response.test.ts tests/seed-cache.test.tsResult: 136/136 passed.
Additional validation:
vp check: passedvp run vinext#build: passed