fix(pages): preload dev hydration modules - #2777
Conversation
commit: |
|
Performance benchmarksCompared 0 improved · 0 regressed · 6 within ±1.5%
View detailed results and traces 🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head |
aff40a9 to
cf5bdf7
Compare
cf5bdf7 to
ebe9962
Compare
|
/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): preload dev hydration modules
Reviewed the full diff, traced the runtime paths, and ran the targeted suites (pages-asset-tags, pages-dev-module-url, entry-templates, build-optimization) — all green, lint clean. The core approach is sound: emitting ordered, parser-visible <script type="module"> bootstrap tags so the initial _app/page namespaces are consumed synchronously and hydrateRoot() installs React's delegated handlers before the document load event. The load-event flyout probe in hydration-extras.spec.ts is a good, targeted regression guard for exactly that claim.
A few things I looked at closely and one maintainability concern worth flagging.
1. Fragile implicit coupling between moduleIds order and initialModules indices (maintainability)
The generated client entry hardcodes the array positions:
pages-client-entry.ts:254—initialModules?.[${hasApp ? 1 : 0}](page)pages-client-entry.ts:266—initialModules?.[0](app)
These indices are only correct because pages-page-handler.ts:1021-1023 builds pageModuleIds as [appAssetPath?, route.filePath] (app first, page second), and collectAssetTags emits initialModule{i} positionally. There is no test that ties these two ends together — entry-templates.test.ts checks the client entry in isolation, and pages-asset-tags.test.ts checks the tag output in isolation. If someone reorders pageModuleIds, or the app-file gating drifts, hydration silently regresses (page renders from the wrong namespace / falls back to async load, reintroducing the exact post-load-event bug this PR fixes) with no unit failure.
Consider one of:
- A comment on both sides pointing at the other (
// index must match pages-page-handler pageModuleIds order), and/or - Emitting a small named map instead of a bare positional array (e.g. keyed by
"app"/"page"), so the consumer isn't index-coupled to producer order.
Not blocking, but this is the kind of implicit contract that tends to break later.
2. hasApp (codegen) vs appAssetPath (runtime) — verified consistent on this path
I checked whether hasApp = appFilePath !== null (client entry) could diverge from the runtime appAssetPath, which would shift the index. On the path that actually consumes this bootstrap (pages-server-entry → pages-page-handler), appAssetPath is the static _appAssetPath (non-null whenever _app exists), so it matches hasApp. Note dev-server.ts:1551 gates differently (AppComponent ? appFilePath : null), but that path does not call collectAssetTags, so no mismatch. Worth being aware of if the two Pages dev paths are ever unified.
3. Double install of the dev error overlay — safe
The bootstrap script calls installDevErrorOverlay() / reportInitialDevServerErrors(), and the client entry calls them again. installDevErrorOverlay is idempotent (dev-error-overlay.tsx:72 if (installed) return), and the bootstrap /@fs/…/dev-error-overlay.tsx URL resolves to the same module id as the vinext/dev-error-overlay alias, so the installed guard is shared. Fine. reactPreamble / instrumentation-client are likewise imported once across the two bootstrap scripts and re-imported (idempotently) by the entry.
4. Injection safety — good
Module URLs are embedded via safeJsonStringify, which escapes </>/&, so dynamic-route bracket paths can't break out of </script>. Dev-only path regardless.
5. optimizeDeps.include for Pages-only projects — reasonable
Pre-including the React framework entrypoints at the top-level client optimizer (gated !hasAppDir && hasPagesDir) mirrors the per-environment configs and addresses the cold-cache optimizer race. mergeStringArrayValues dedupes via Set. No concern.
6. createPagesDevModuleUrl @fs/ fallback
The out-of-root branch produces /<base>/@fs//abs/path (double slash after @fs on POSIX, since the absolute path starts with /). That's the standard Vite /@fs/ convention and the test asserts it explicitly (pages-dev-module-url.test.ts:32), so it's intended.
Verdict
Approach and implementation look correct; the fix is well targeted and the enabled E2E specs are appropriate. My only real ask is (1) — add a guard/comment or a keyed map to make the moduleIds ⇄ initialModules index contract explicit, since it's currently an untested cross-file invariant that silently reintroduces the bug if broken.
|
Review posted to PR #2777. SummaryI reviewed the full diff, traced both Pages Router dev paths, and ran the targeted suites ( The approach is sound. Emitting ordered parser-visible Findings:
Verdict: approach and implementation are correct; only ask is to make the index contract explicit. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: fix(pages): preload dev hydration modules
Re-reviewed after 5e94131 (fix(pages): key dev hydration modules by role), which directly addresses the one real ask from my prior review. Traced both Pages dev paths again, re-ran the targeted suites (pages-asset-tags, pages-dev-module-url, entry-templates, build-optimization — 247 passed / 3 skipped) and vp check on the four changed source files (format + lint + types all clean).
Prior finding (fragile positional index contract) — resolved
5e94131 replaces the positional initialModules?.[hasApp ? 1 : 0] / [0] coupling with a role-keyed map:
- Producer:
pages-page-handler.ts:1027-1030passesdevInitialModules: { app: appAssetPath, page: route.filePath }, decoupled frompageModuleIdsordering. - Emitter:
pages-asset-tags.ts:207-228iterates["app","page"]and emitswindow.__VINEXT_INITIAL_PAGES_MODULES__ = { app, page }by name. - Consumer:
pages-client-entry.ts:254,266readsinitialModules?.page/initialModules?.app.
Crucially there is now a test that ties both ends together: pages-asset-tags.test.ts "primes development page modules before the shared client entry" deliberately passes moduleIds in the reverse order of devInitialModules and asserts each role imports the correct namespace, and entry-templates.test.ts adds expect(code).not.toContain("initialModules?.["). Reordering either side would now fail a unit test. This is exactly the guard I asked for.
Correctness spot-checks (all pass)
-
Key consistency for
devModuleUrlslookups.devModuleUrlsis keyed bytoSlash(moduleFilePath)inindex.ts:3790, while the handler looks up byroute.filePath/appAssetPath. Both originate from the samepagesRouterscan andfindFileWithExts(pagesDir, "_app", ...)call, and per the repo'spathslashinvariant those ids are already canonical forward-slash — so the lookups hit. No mismatch. -
hasApp(codegen) vsappAssetPath(runtime). On the bootstrap-consuming path (pages-server-entry→pages-page-handler),appAssetPathis the static_appAssetPathderived from the sameappFilePath !== nullcheck that setshasApp, so app-presence stays in sync andinitialModules.appis only read when theappLoaderbranch is generated. When no_app,moduleRefs.appstays"null"and the client entry never reads it. -
Module-execution ordering. The two bootstrap
<script type="module">tags are pushed before the client-entry tags, and ES module scripts execute in document order; static imports in the second bootstrap script run before its body setswindow.__VINEXT_INITIAL_PAGES_MODULES__, so the namespaces exist by the time the client entry reads the global. Import ordering (instrumentation → react preamble → overlay) matches the client entry. -
Idempotency of double imports.
dev-error-overlay(installedguard),instrumentation-client(state overwrite), and the react preamble are all safe to import from both the bootstrap and the client entry — browser module caching runs each body once per resolved URL, and the state writes are overwrite-idempotent. -
base/assetPrefixseparation.createPagesDevModuleUrl/createPagesDevAssetUrlnow thread Vitebaseand the "keeps development modules on Vite's base instead of assetPrefix" test assertsassetPrefix(cdn.example.com) never leaks into dev module URLs. The/@fs/out-of-root fallback is covered bypages-dev-module-url.test.ts. -
Injection safety. All embedded URLs go through
safeJsonStringify; dynamic-route bracket paths can't break out of</script>. Dev-only regardless.
Verdict
Approve. The role-keyed refactor closes the only outstanding concern with a real cross-end test, and the rest of the implementation holds up under scrutiny. Reminder from the PR body: retarget to main after #2775 merges (base is currently codex/fix-pages-raw-data-middleware).
|
Review posted to PR #2777. SummaryI re-reviewed the PR after the new commit Prior finding resolved. The previous review's one real ask — the fragile untested positional-index contract between Correctness spot-checks (all pass):
Verdict: Approve. No blocking issues found. The one prior concern is fully closed with a real cross-end test. Reminder (from PR body): retarget to |
5e94131 to
b75e10a
Compare
|
/bigbonk review for issues |
|
/bigbonk review for issues |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Re-review: fix(pages): preload dev hydration modules (+ hash-only navigation)
The PR now targets main directly (#2775 merged). Since the two prior ask-bonk reviews, two new commits landed that I focused this pass on:
ea6b401fix(link): preserve hash-only Pages navigation state86e3183fix(router): keep changed hashes on the hash-only path
Re-ran the targeted suites — link (130), pages-asset-tags (38), pages-dev-module-url (9/1 skip), entry-templates (52), build-optimization (151/2 skip) — all green, plus vp check on the 7 changed source files (format + lint + types clean).
isHashOnlyBrowserUrlChange — verified against Next.js onlyAHashChange
The core of 86e3183. I diffed it against the authoritative onlyAHashChange in packages/next/src/shared/lib/router/router.ts:2337:
if (newHash && oldUrlNoHash === newUrlNoHash && oldHash === newHash) return true
if (oldUrlNoHash !== newUrlNoHash) return false
return oldHash !== newHashvinext's rewrite keeps the #-delimiter distinction (absent vs empty fragment) that URL.hash erases, then mirrors the same three branches: same non-empty hash → true (still scrolls), differing pathname/search → false, otherwise oldHash !== newHash. I ran the full truth table (absent/empty/same/diff hash across /p, /p#, /p#a, /p#b) and every case matches Next.js. The <Link href="#"> no-op-remount fix is correct.
Two minor semantic notes (neither a bug):
- Next.js compares
oldUrlNoHash/newUrlNoHashwhich include the query string; vinext splits pathname and search and requires both to match — equivalent. - Next.js uses
split('#', 2)(truncates at the first#), while vinext'sslice(hashIndex + 1)keeps the remainder of a multi-#fragment. For#a#bthe extracted hash differs (avsa#b), but both still classify it as a hash-only change, so the observable result is identical. Not worth changing.
SSR-time hash resolution (ea6b401) — correct and safe
- The new
pages-router-ssr-context.tsbridgesnext/linkand the server-only router state via aSymbol.foraccessor, which is the right pattern given RSC/SSR runnext/routerandnext/linkas separate module instances.getPagesRouterSSRContext()returnsnullin the browser (typeof window !== "undefined"guard), so it only feeds SSR rendering; the browser path still useswindow.next.router.asPath. SSRContext(asPath: string; locales?: string[]) is structurally compatible withPagesRouterSSRContext, and the accessor is request-scoped through the existing AsyncLocalStorage state.resolvePagesQueryOnlyHrefremains gated onHAS_PAGES_ROUTER, so App-Router state can't leak in.- The
Router = pagesRouter ?? window.next?.routerchange (link.tsx:1514) now prefers the mountedRouterContextvalue. I confirmed that value is built bybuildRouterValue(...)in router.ts:3412 with fullpush/replace/back/forward/reload/prefetch/beforePopStatedelegating to the singleton, and the"reload" in Routerguard still holds — so navigation methods remain intact. Preferring the context matches Next.jsLink. resolvePagesLinkNavigationHref(link.tsx:1414) callsresolvePagesQueryOnlyHrefwithout an explicit context, but it only runs inside the click handler (browser), where thewindow.next.routerfallback covers it. Consistent.
Fixture regression guard
data-rendered-at-ms={lifted.renderedAtMs} (gallery) plus the enabled shallow-routing spec asserting the timestamp is unchanged after a sort change is a good, direct proof that gSSP is not re-run on shallow navigation. lifted is correctly un-Omitted from GalleryWallComponentProps and PageLiftedProps.renderedAtMs: number exists.
Prior findings
Both earlier concerns remain resolved: the dev-hydration role-keyed map ({ app, page }) with its cross-end reverse-order test, and the base/assetPrefix separation. Nothing regressed.
Verdict
Approve. The two new commits are well-targeted; the hash-only classifier is a faithful port of Next.js onlyAHashChange (verified against source + full truth table), the SSR-context bridge is sound and browser-safe, and the enabled E2E specs are appropriate regression guards. No blocking issues.
|
Review posted to PR #2777. SummaryThe PR now targets
Verdict: Approve. No blocking issues. Key verifications:
Both prior concerns (role-keyed dev-hydration map, |
1 participant
Summary
_appand page modules through ordered parser-visible development scriptsbase//@fs/, independent of productionassetPrefixhistory.replaceState, shallow routing, and router eventsThese six failures had one framework root cause: development hydration began after the document load event because the shared Pages client entry dynamically imported the initial modules. Router/history symptoms were knock-on effects of controls being exposed before hydration, not separate routing bugs.
This PR is temporarily stacked on #2775 because timely hydration exposes that PR's independent raw
/_next/datamiddleware bug in an existing full-navigation assertion. Retarget tomainafter #2775 merges.Next.js parity references:
Validation