fix(pages): preserve fast refresh state - #2544
Conversation
|
/bigbonk review for issues |
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 |
![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): preserve fast refresh state
Reviewed the full diff against main. The core mechanism is sound and the change matches Next.js Pages Router Fast Refresh behavior. Summary of what I verified and a few minor notes.
What the change does (verified correct)
The old hotUpdate hook force-sent full-reload for _app/page/script edits, wiping browser state. The new approach:
- Converts
hotUpdateto the object form withorder: "post"so it runs after@vitejs/plugin-react, letting client React Fast Refresh handle the client update. - For the SSR environment only, it manually invalidates the changed modules and
return [], which empties the SSR env's module list. This is the key trick: without it, Vite'supdateModuleswould hit a dead-end boundary on the SSR page module and fall back to afull-reload(see vite-plus-coreupdateModules->needFullReload). By consuming the SSR modules post-hook, thehmr()loop early-returns with "no modules matched" and no reload is sent. The manualinvalidateModule(mod, new Set(), timestamp, true)mirrors Vite's own invalidation call, so the SSR entry re-renders fresh HTML on the next request. - Route add/delete still triggers full reloads for all three modes: pages-only (
server.ws.sendat theadd/unlinkwatcher handlers), hybrid (invalidateHybridClientEntries), and app-only (invalidateRscEntryModule). Only content edits are now state-preserving. This matches Next.js.
Parity check
_appedits: previously forced full-reload; now preserved.isPotentialPagesAssetGraphScriptreturns true for_app.tsx(it's a script under root, outsideappDir), so the SSR-invalidation branch fires for it. Correct.- Stylesheet edits:
pagesAssetGraphChangedinvalidates client assets, butpagesAssetGraphScriptChangedis false, so the SSR branch is skipped and Vite's CSS HMR handles it. Correct. pagesRunner.clearCache(): the runner cache is still cleared for every relevant script edit via the watcherchangehandler'sinvalidatePagesClientAssetsModule()(thepagesAssetGraphScriptChangedcondition matches). No stale-SSR risk introduced. Correct.- Dev-only concern; prod-server / worker-entry are untouched, so dev/prod parity holds.
Test results
vp check packages/vinext/src/index.ts tests/pages-router.test.ts— clean (format, lint, types).- New unit test
does not force full reload for Pages Router Fast Refresh updatespasses and asserts theorder: "post", nofull-reloadon script/app/shared edits, the SSRreturn []+invalidateModule(..., true), and the route add/unlink reloads. Good coverage. - The one failing test in the full
tests/pages-router.test.tsrun is a pre-existingECONNREFUSED 127.0.0.1:5173connection error unrelated to this change (sandbox networking), not a regression from this PR.
Minor notes (non-blocking)
- E2E test mutates a committed fixture (
tests/fixtures/pages-basic/pages/hmr-state.tsx). It's restored infinally, but if the process is hard-killed mid-run the fixture is left dirty. This is an accepted pattern for HMR tests, but worth being aware of when running the suite locally. page.waitForTimeout(1_000)in the syntax-error recovery step is a fixed sleep and could be flaky on slow CI. Next.js's own error-recovery util uses a similar wait, so this mirrors upstream, but consider replacing with a poll on the error overlay disappearing if it flakes.- New static fixture page
hmr-state.tsxis consistent with the many existing static pages inpages-basicand does not appear in any exhaustive route/snapshot assertion, so it should not break other suites.
Overall this is a clean, well-scoped fix with good test coverage. LGTM pending CI (full Vitest + Playwright).
|
Review posted to PR #2544. SummaryI reviewed PR #2544 ( Core mechanism (verified correct):
Parity verified: Tests: Minor non-blocking notes flagged in the review: the e2e test mutates a committed fixture (restored in Overall: LGTM pending CI. |
|
/bigbonk review for issues |
8e2b5e6 to
6cfbb21
Compare
|
/bigbonk review for issues |
6cfbb21 to
0e81996
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): preserve fast refresh state
Reviewed the full diff (both commits 1627925 and 8e2b5e6) against main, traced the Vite HMR internals, and ran the targeted tests. The core Fast Refresh mechanism is correct. Findings below, ordered by importance.
Core mechanism — verified correct
The hotUpdate hook is now object form with order: "post", and the SSR branch returns [] after manually invalidating changed modules. I traced this against vite-plus-core's hmr()/updateModules():
- Non-client environments: the returned
[]replaceshot.options.modules, sohmr(ssrEnv)hits!options.modules.lengthand early-returns with[no modules matched]— nofull-reload, because the reload in that branch is gated onenvironment.name === "client"(node.js:32470-32481). ✅ - Client environment: vinext's
hotUpdatereturnsundefined(SSR branch only fires whenthis.environment?.name === "ssr"), so client modules are untouched and@vitejs/plugin-react(which runs before, since vinext isorder: "post") handles React Fast Refresh. ✅ this.environmentbinding: confirmed each per-environment call usesenvironment.pluginContainer.minimalContext, whose.environmentis that environment (node.js:36042-36047). Sothis.environment?.nameresolves correctly per call. ✅- Route add/delete full reload moved from the old
hotUpdatebranch to theadd/unlinkwatcher handlers (index.ts:4285,4321), gated on!hasAppDir. The new unit test asserts both fire{ type: "full-reload" }. ✅
vp check is clean on all changed files; the new unit test (does not force full reload...) and the new CSS test both pass. All existing dev Pages CSS tests (including CSS-module cases) still pass.
Scope: second commit is undocumented
Commit 8e2b5e6 "fix(pages): preload transformed dev styles" adds collectTransformedStylesheetAssets (+117 lines incl. test) and is not mentioned in the PR summary/motivation/validation. It's a distinct dev-CSS fix bundled into a Fast Refresh PR. Please either (a) add it to the PR description so reviewers know what they're approving, or (b) split it into its own PR. Right now the described validation section doesn't cover it.
Non-blocking notes
-
Hand-rolled import regex is fragile (
dev-server.ts:166-167)./(?:import|export)\s*(?:[^"']*?from\s*)?["']([^"']+)["']/giwill also match string literals that merely look like imports and anyimport(...)/export ... fromforms. It runs on Vite-transformed client code (mostly trusted, rewritten specifiers), so false positives are unlikely to produce real hrefs, but this is a maintenance hazard. Consideres-module-lexer(already common in Vite land) instead of a regex, or at least a comment noting the transformed-code assumption. -
Per-request recursive graph walk in dev (
collectTransformedStylesheetAssets). This runs on every dev SSR render and recursivelytransformRequests + regex-scans every/-prefixed client import transitively reachable from_app+ the page.transformRequestis cached, but the traversal and regex scan repeat each request and grow with app size. Dev-only, so acceptable, but worth a follow-up if large apps see slow dev renders. TheseenModulesguard correctly prevents infinite recursion. -
E2E test mutates a committed fixture (
tests/fixtures/pages-basic/pages/hmr-state.tsx). Restored infinally, but a hard kill mid-run leaves it dirty. Accepted pattern for HMR tests; noting for local runs. -
page.waitForTimeout(1_000)in the syntax-error recovery step is a fixed sleep and could flake on slow CI. Next.js's own recovery util uses a similar wait, so it mirrors upstream — consider polling on the error overlay disappearing if it flakes.
Parity
Dev-only change; prod-server.ts / worker-entry.ts untouched, so dev/prod parity holds. The second commit narrows a dev-only FOUC gap rather than introducing dev-only styles that would be absent in prod.
Overall: core fix is solid and well-tested. Main ask is documenting/splitting the second commit; the regex parser and per-request walk are non-blocking but worth a follow-up. LGTM pending CI.
|
I completed a thorough review of PR #2544 and posted it. Summary of what I found: Verdict: LGTM pending CI — core fix is correctCore mechanism (verified against Vite internals):
Most important finding — undocumented scope creep:
Non-blocking notes flagged:
Dev/prod parity holds (prod-server and worker-entry untouched). |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): preserve fast refresh state
Reviewed both commits (1627925 preserve fast refresh state, 0e81996 preload transformed dev styles) against main, traced the Vite HMR/CSS-graph internals, and ran the targeted tests. The core mechanism is correct and I found no blocking issues. Notes below, ordered by importance.
Core Fast Refresh mechanism — verified correct
hotUpdateis now object form withorder: "post", running after@vitejs/plugin-react, so client React Fast Refresh handles the client update instead of vinext forcing afull-reload.- The SSR branch (
this.environment?.name === "ssr") manually invalidates the changed modules withinvalidateModule(mod, new Set(), timestamp, true)andreturn []. Returning[]empties the SSR env's module list so Vite doesn't hit a dead-end boundary and fall back tofull-reload. Because the SSR environment does not push HMR updates to the browser (only the client env does), the manual invalidation is behaviorally equivalent to Vite's own SSR invalidation — the SSR entry re-renders fresh HTML on the next request. ✅ - Route add/delete full-reload was correctly relocated to the
add/unlinkwatcher handlers. Parity holds across all three modes: pages-only via the newif (!hasAppDir) server.ws.send({ type: "full-reload" })(index.ts:4285,4321), hybrid viainvalidateHybridClientEntries()(which sendsfull-reload,index.ts:4118), and app-only viainvalidateRscEntryModule(). ✅
Hybrid-mode SSR return [] — checked, not a defect
I specifically investigated whether the SSR return [] could swallow App Router Fast Refresh in hybrid apps. isPotentialPagesAssetGraphScript returns true for any script under root outside appDir (e.g. a shared lib/*.ts imported by an app-router client component), so the SSR branch fires for those files in hybrid mode too. However, App Router HMR is driven by the RSC plugin + client re-fetch (app-browser-navigation-controller.ts), not by the Pages plugin's SSR hotUpdate result, and the SSR env never pushes updates to the browser. The manual invalidateModule(..., true) still invalidates the shared module for the SSR env, so App Router re-renders fresh on the next request. No parity regression. Worth adding a hybrid HMR unit test to lock this in, since the current unit test only exercises pages-only mode.
Second commit (transformed dev styles) — now documented, verified
collectTransformedStylesheetAssetswalks the client module graph (transformRequest+moduleGraph.getModuleByUrl→importedModules) to discover CSS introduced by client transforms (including\0-prefixed virtual CSS, re-encoded as/@id/__x00__...). This mirrors what production discovers via the build's SSR manifest (prod-server.ts:1199), so dev/prod first-paint parity holds. ✅try/catcharound the walk preserves the source-manifest fallback if a third-party client transform throws while the server render is still valid. Good defensive choice.- Dedup against
manifestHTMLviahtml.includes(href="...")is sound for identical hrefs.
Non-blocking notes
-
Duplicate
<link>risk when hrefs differ for the same CSS. The manifest path emitscreatePagesDevAssetUrl(file)while the transformed path may emit a rawimportedModule.urlor/@id/__x00__.... If the same underlying stylesheet surfaces under two different URLs, thehtml.includes(href=...)dedup won't catch it and the page loads the CSS twice. Dev-only, cosmetic (no FOUC), but worth a follow-up if it shows up. -
Per-request recursive client-graph walk.
collectTransformedStylesheetAssetsruns on every dev SSR render and recursivelytransformRequests + scans the transitive client graph from_app+ the page. ThecacheKeycache mitigates repeat cost, and it's fully invalidated on anyadd/change/unlink— but the cache clears on every file change (not just relevant ones), so an app with frequent unrelated edits re-walks each time.seenModulescorrectly guards recursion. Dev-only, acceptable; follow-up candidate for large apps. -
CSS discovery only recurses into
type: "js"importers. Modules with other/undefined types that transitively import CSS would be skipped. Unlikely in practice for Pages Router, noting for completeness. -
Watcher listeners aren't removed on
server.close(). Theadd/change/unlinkcache-clear listeners registered incollectTransformedStylesheetAssetspersist for the server's lifetime (they close over the per-servercacheMap). WeakMap/WeakSet keying means it's GC'd with the server, so no real leak — noting for awareness. -
E2E test mutates a committed fixture (
tests/fixtures/pages-basic/pages/hmr-state.tsx, restored infinally) and uses a fixedpage.waitForTimeout(1_000)in the syntax-error recovery step. Both mirror Next.js's own HMR test utilities; consider polling on the error overlay disappearing if the sleep flakes on slow CI.
Verification
vp check packages/vinext/src/index.ts packages/vinext/src/server/dev-server.ts tests/pages-router.test.ts— clean (format, lint, types).- New unit test
does not force full reload for Pages Router Fast Refresh updates— passes; assertsorder: "post", nofull-reloadon shared/_app/page edits, the SSRreturn []+invalidateModule(..., true), and the route add/unlink reloads. - New test
dev Pages client assets expose virtual CSS added by client transforms— passes. - All 8 existing
dev Pagesstylesheet tests (incl. CSS-modules, ISR, custom error, transitive imports) — pass.
Dev-only change; prod-server.ts / worker-entry.ts untouched. Overall: solid, well-tested fix. Main suggestion is a hybrid-mode HMR unit test; the rest are non-blocking follow-ups. LGTM pending CI.
|
Review posted to PR #2544. SummaryI reviewed both commits of PR #2544 end-to-end, traced the Vite HMR and CSS-graph internals, and ran the targeted tests. The core fix is correct — no blocking issues. Verified correct:
Investigated but not a defect: the hybrid-mode SSR Second (transformed-CSS) commit: walks the client module graph to discover transform-injected/virtual CSS, mirroring what prod discovers via the build's SSR manifest — dev/prod first-paint parity holds. Non-blocking follow-ups flagged: possible duplicate
|
0e81996 to
a23f701
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): preserve fast refresh state
Reviewed both commits (1627925 preserve fast refresh state, a23f701 preload transformed dev styles) against main, traced the vite-plus-core HMR + CSS-graph internals, and ran the targeted tests. The core mechanism is correct — no blocking issues. Findings ordered by importance.
Core Fast Refresh mechanism — verified correct against vite-plus-core internals
I traced the object-form hotUpdate (order: "post") through vite-plus-core hmr():
- The hook runs in two separate loops (
node.js:32430client-only,node.js:32453per non-client env), each called with that environment'spluginContainer.minimalContext, sothis.environment?.nameresolves correctly per call. - Client env: vinext returns
undefined(SSR branch guarded byname === "ssr"), soclientHotUpdateOptions.modulesis preserved and@vitejs/plugin-react(runs before, since vinext isorder:"post") handles React Fast Refresh. ✅ - SSR env: returns
[]→hot.options.modules = [](node.js:32460) →hmr()hits!options.modules.lengthand thefull-reloadthere is gated onfile.endsWith(".html") && environment.name === "client"(node.js:32471), so a.tsxSSR edit only logs[no modules matched]— no reload. The manualinvalidateModule(mod, new Set(), timestamp, true)matches Vite's own call inupdateModules(node.js:32504), so SSR re-renders fresh HTML on the next request. ✅ - RSC env (hybrid): returns
undefined, so RSC modules are preserved and the RSC plugin drives App Router HMR. TheinvalidateModule(..., true)still invalidates shared modules for the SSR env, so hybrid App Router pages re-render fresh. No parity regression. ✅ - Route add/delete full-reload was correctly relocated to the
add/unlinkwatcher handlers, gated on!hasAppDir(index.ts:4285,4321). Hybrid usesinvalidateHybridClientEntries(); app-only usesinvalidateRscEntryModule(). ✅
vp check is clean on all changed files. New unit tests (does not force full reload for Pages Router Fast Refresh updates, does not force full reload for shared App Router code in hybrid apps) and the CSS test (dev Pages client assets expose virtual CSS added by client transforms) all pass, as do all 8 existing dev Pages stylesheet tests. Good coverage — and the hybrid-mode unit test suggested in the prior review round is now present.
Behavior-change note (non-blocking) — non-script Pages extensions
The old hotUpdate had a pages-only client-env fallback (options.file.startsWith(pagesDir) && fileMatcher.extensionRegex.test(options.file) → full-reload) that covered non-script page extensions (e.g. .mdx, .md). The new handler only special-cases scripts (isScriptModuleId → .[cm]?[jt]sx?), CSS, and _app. For an .mdx page edit in pages-only mode, vinext no longer participates, so Vite's default HMR applies — which is fine when the MDX plugin produces a Fast-Refresh-accepting JS module, but if the SSR page module hits a dead-end boundary Vite will fall back to full-reload (acceptable, matches old behavior). Worth a quick confirmation that MDX Pages Router HMR still behaves, since there's no test for non-script page extensions.
Transformed dev CSS collector — non-blocking follow-ups
collectTransformedStylesheetAssets (dev-server.ts:154) walks the client module graph to discover transform-injected/virtual CSS, mirroring prod's SSR-manifest discovery. Dev/prod first-paint parity holds. A few follow-ups:
- Duplicate
<link>when the same CSS surfaces under different URLs. Manifest path emitscreatePagesDevAssetUrl(file); transformed path emits rawimportedModule.urlor/@id/__x00__.... Thehtml.includes(href="...")dedup (dev-server.ts:241) only catches identical hrefs, so the same stylesheet reachable under two URL forms would load twice. Dev-only, cosmetic. - CSS discovery only recurses into
type: "js"importers (dev-server.ts:195). Modules with other/undefined.typethat transitively import CSS are skipped. Unlikely for Pages Router; noting for completeness. - Per-request recursive client-graph walk on every dev SSR render. The
cacheKeycache mitigates repeat cost but is cleared on everyadd/change/unlink(dev-server.ts:166-169), not just relevant ones, so an app with frequent unrelated edits re-walks each time.seenModulescorrectly guards recursion. Dev-only; follow-up candidate for large apps. - Watcher listeners aren't removed on
server.close()— they close over the per-servercacheMap keyed by WeakMap/WeakSet, so they're GC'd with the server. No real leak; noting for awareness.
E2E test — non-blocking
hmr.spec.ts mutates a committed fixture (tests/fixtures/pages-basic/pages/hmr-state.tsx, restored in finally) and uses a fixed page.waitForTimeout(1_000) for syntax-error recovery. Both mirror Next.js's own HMR test utilities. Note that pages-router runs all ~24 spec files against one shared vp dev server; the mutation triggers a server-wide watcher change (clears the CSS cache + invalidates the pages-client-assets module), but hmr-state is a dedicated route no other test loads, so cross-test interference is negligible. Consider polling on the error-overlay disappearing instead of the fixed sleep if it flakes on slow CI.
Parity
Dev-only change; prod-server.ts / worker-entry.ts untouched, so dev/prod parity holds. The CSS commit narrows a dev-only FOUC gap rather than introducing dev-only styles absent in prod. The __HMR_MARKER__ assertion in the E2E test correctly guards the fix (a full reload would wipe the marker).
Overall: solid, well-scoped, well-tested fix, and the scope-creep/hybrid-test concerns from prior rounds are now addressed. LGTM pending CI (full Vitest + Playwright).
|
Review posted to PR #2544. SummaryI reviewed both commits of PR #2544 ( Verified correct against Vite internals:
Tests: Notes flagged (all non-blocking):
The scope-creep concern from earlier rounds is resolved — the PR body now documents both the Fast Refresh and CSS-preload changes. LGTM pending CI. |
|
/bigbonk review for issues |
a23f701 to
4c4f440
Compare
|
I think
is wrong, but the solution to follow vite's CSS resolving behaviour sounds correct to me. Thanks for looking into it |
There was a problem hiding this comment.
Review: fix(pages): preserve fast refresh state
Reviewed both commits against main, traced the vite-plus-core HMR/CSS internals, and ran the targeted Vitest files. Core mechanism is correct — no blocking issues. One correction to prior review rounds, plus a couple of non-blocking notes.
Correction to the "full-reload is gated on client env" claim
Prior review rounds asserted the SSR return [] avoids a reload because the full-reload in hmr() is gated on environment.name === "client". That gate (node.js:32471) only applies to the !options.modules.length early-return path. When options.modules.length > 0, hmr() calls updateModules(), whose full-reload send at node.js:32525 (if (needFullReload || isClientHtmlChange)) is not gated on the client env — it fires for any environment on a dead-end boundary.
The reason this PR is still correct is different: the SSR environment's hot is a noop channel (node.js:30432 — only the client env carries the WS transport), so an SSR-env full-reload never reaches the browser regardless. The actual fix here is removing the explicit options.server.ws.send({ type: "full-reload" }) / client.hot.send(...) calls (which targeted the real browser WS) from the old hotUpdate. The return [] + manual invalidateModule(mod, new Set(), timestamp, true) is then just to (a) skip the SSR updateModules walk and (b) hard-invalidate the SSR module so the next request re-renders fresh HTML. I verified the 4th positional arg (isHmr=true) matches Vite's own updateModules invalidation at node.js:32504. This mechanism is sound.
MDX path — verified, not a defect
The new reactOptions.include: /\.(?:[tj]sx?|mdx)$/ is a strict superset of @vitejs/plugin-react's default /\.[tj]sx?$/ (adds .mdx, loses nothing), so React Fast Refresh now applies to compiled MDX. For .mdx edits, isPotentialPagesAssetGraphScript is false (not a script id), so the SSR return [] branch does not fire — but that's fine: the SSR-env updateModules full-reload goes to the noop channel, and the client env's refresh-transformed MDX module self-accepts. So MDX state is preserved, matching the new E2E assertion. @mdx-js/rollup is hoisted in the workspace, so the hmr-mdx.mdx fixture builds via vinext's auto-detected MDX proxy.
Non-blocking notes
-
Possible duplicate
<link>for the same CSS under differing URL forms. The manifest path emitscreatePagesDevAssetUrl(file)whilecollectTransformedStylesheetAssetsemits the rawimportedModule.url(or/@id/__x00__...). Thehtml.includes(href="...")dedup (dev-server.ts:241) only catches identical hrefs, so a stylesheet reachable under two URL forms would emit twice. Dev-only, cosmetic (no FOUC). Existing stylesheet tests use.toContain, so they won't catch a duplicate. -
CSS discovery only recurses into
type: "js"importers (dev-server.ts:195). Modules withnull/other.typethat transitively import CSS are skipped. Unlikely for Pages Router; noting for completeness. -
Per-request recursive client-graph walk, cleared over-eagerly.
collectTransformedStylesheetAssetsruns on every dev SSR render; thecacheKeycache mitigates repeat cost but is cleared on everyadd/change/unlink(dev-server.ts:166-169), not just relevant edits.seenModulescorrectly guards recursion. Dev-only; follow-up candidate for large apps. Its watcher listeners aren't removed onserver.close(), but WeakMap/WeakSet keying means they GC with the server — no real leak. -
E2E test mutates committed fixtures (
hmr-state.tsx,hmr-mdx.mdx, restored infinally) against the sharedlocalhost:4173dev server, and uses a fixedpage.waitForTimeout(1_000)for syntax-error recovery. Both mirror Next.js's own HMR utilities; consider polling on the error-overlay disappearing if the sleep flakes on slow CI.
Verification
vp check packages/vinext/src/index.ts packages/vinext/src/server/dev-server.ts— clean.- New unit test
does not force full reload for Pages Router Fast Refresh updatesanddev Pages client assets expose virtual CSS added by client transforms— pass. - All 8 existing
dev Pagesstylesheet tests (incl. CSS-modules, ISR, basePath, transitive) — pass; the basePath test confirms the transformed collector doesn't break non-root-base hrefs. tests/next-config.test.ts+tests/routing.test.ts(325 tests) — pass, so thepageExtensions: [...,"mdx"]addition to the sharedpages-basicfixture doesn't perturb route discovery.
Dev-only change; prod-server.ts / worker-entry.ts untouched, so dev/prod parity holds. LGTM pending CI (full Vitest + Playwright).
|
Review posted to PR #2544. SummaryI reviewed both commits of PR #2544 ( Most important finding — a correction to prior review roundsEarlier The PR is still correct, but for a different reason: the SSR environment's Other verified items
Non-blocking notes flagged
|
|
/bigbonk review for issues |
4c4f440 to
8e1e504
Compare
There was a problem hiding this comment.
Review: fix(pages): preserve fast refresh state
Reviewed both commits against main, ran the targeted Vitest files, and traced the @vitejs/plugin-react split. Found one CI-blocking regression that prior /bigbonk rounds missed, plus context on the new React/MDX plugin restructuring that the PR body doesn't mention.
Blocking — existing test registers vinext:mdx proxy plugin now fails
This PR splits the old single vinext:mdx proxy plugin into two: vinext:mdx (transform only) and a new vinext:mdx-config (config only). But the pre-existing, unmodified test still asserts that vinext:mdx carries the config hook:
tests/pages-router.test.ts:4056
expect(typeof mdxProxy.config).toBe("function"); // ← now undefined
Reproduced in isolation:
$ vp test run tests/pages-router.test.ts -t "vinext:mdx"
FAIL Plugin config > registers vinext:mdx proxy plugin with enforce pre for correct ordering
AssertionError: expected 'undefined' to be 'function'
4056| expect(typeof mdxProxy.config).toBe("function");
The config hook moved to vinext:mdx-config, so plugins.find(p => p.name === "vinext:mdx").config is now undefined. This test passed on main and is not touched by the PR, so the Vitest required check will fail. Fix: update the assertion to look up vinext:mdx-config for the config hook (or point the test at the new plugin), e.g.:
const mdxConfig = plugins.find((p) => p.name === "vinext:mdx-config");
expect(typeof mdxConfig.config).toBe("function");
expect(mdxConfig.config({}, { command: "build", mode: "production" })).toBeUndefined();Every prior review round in this thread reported "vp check clean; all tests pass," but none of them ran this specific existing test — vp check only type/lint/format-checks, and the new-test runs were name-filtered to the Fast Refresh / CSS tests. Worth running the full Plugin config describe block (vp test run tests/pages-router.test.ts -t "Plugin config") before merge.
Undocumented scope: React plugin is now instantiated twice
The PR body only describes Fast Refresh state + transformed CSS. It does not mention that reactPluginPromise now:
- instantiates
@vitejs/plugin-reacttwice — abuildset with the configured options and aserveset that adds.mdxtoinclude— and wraps each plugin in alimitToCommand(...)applyguard (index.ts:1511-1540); - splits the MDX proxy into
vinext:mdx+vinext:mdx-config(index.ts:1617-1649).
I traced the apply filtering and it is correct:
vite:react:refresh-wrapper(apply: "serve") is excluded from the build set ("serve" === "build"→ false) and kept in serve. ✓undefined-apply plugins (e.g.vite:react-babel) run in both commands. ✓- The existing
configResolvedduplicate-detection (index.ts:3326, "Duplicate @vitejs/plugin-react detected") does not false-positive, becauseconfig.pluginsis alreadyapply-filtered per command — verified via-t "react"(the double-registration test still passes).
But this is a meaningful architectural change (doubling the react plugin set to give dev an MDX include) bundled into a "preserve fast refresh state" PR. Please document it in the PR body so reviewers know what they're approving, and confirm it's the intended approach vs. simply passing a merged include to a single react instance.
Core Fast Refresh mechanism — verified correct
hotUpdate→ object form withorder: "post", SSR branch manually invalidates (invalidateModule(mod, new Set(), timestamp, true), matching Vite's own call) andreturn []. The explicit browserws.send({ type: "full-reload" })calls were removed and relocated to theadd/unlinkwatchers gated on!hasAppDir(index.ts:4313,4349). Route add/delete still full-reloads; content edits are state-preserving. ✓- New E2E
hmr.spec.tscovers state preservation, syntax-error recovery, and — good — an MDX HMR case (hmr-mdx.mdx), which addresses the non-script-extension gap flagged in earlier rounds. - The new HMR unit tests (pages-only + hybrid shared-code) and the transformed-CSS test all pass.
Non-blocking notes
- False failure I hit while testing:
vp test run ... -t "renders"reports aStatic exportfailure (blog/hello-world.htmlmissing). That's a shared-state artifact — the name filter skips the setup test that populatesexportDir. Running-t "Static export"passes all 7. Not a regression; noting so it isn't mistaken for one. - Transformed-CSS collector (
dev-server.ts:154): still carries the previously-flagged follow-ups — possible duplicate<link>when the same CSS surfaces under differing URL forms (html.includes(href=...)only dedups identical hrefs), recursion only intotype: "js"importers, and the per-request graph walk whose cache is cleared on everyadd/change/unlinkrather than relevant edits only. All dev-only. - E2E mutates committed fixtures (restored in
finally) and uses a fixedpage.waitForTimeout(1_000)for syntax-error recovery — consider polling the overlay if it flakes.
Verdict
Core fix is sound, but not mergeable as-is: the vinext:mdx config-hook assertion is a real, reproducible Vitest failure introduced by the plugin split. Update that test (and document the React/MDX plugin restructuring), then LGTM pending CI.
|
Review posted to PR #2544. SummaryI reviewed both commits of PR #2544 end-to-end, ran the targeted Vitest files, and traced the Blocking finding — an existing test now failsThe PR splits the old single This test passed on Other notable findings
Verdict: core fix is sound but not mergeable as-is; the |
8e1e504 to
40e9096
Compare
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): preserve fast refresh state
Reviewed both commits (fast refresh state + preload transformed dev styles) against main, verified the previously-flagged blocking regression is fixed, and ran the targeted Vitest files. No blocking issues — the core mechanism is correct and the prior-round regression is resolved.
Prior-round blocking regression is fixed
The last /bigbonk round reported that the pre-existing test registers vinext:mdx proxy plugin with enforce pre for correct ordering would fail because the PR split vinext:mdx into vinext:mdx (transform) + vinext:mdx-config (config). That test has now been updated (tests/pages-router.test.ts:4050-4071) to assert mdxProxy.config is undefined and mdxConfigProxy.config is a function. I ran the full Plugin config describe block and it passes (15 tests, incl. the vinext:mdx ordering + lazy-compile cases). The React/MDX plugin restructuring is also now documented in the PR body.
Core Fast Refresh mechanism — verified correct
hotUpdateis object-form withorder: "post", running after@vitejs/plugin-reactso client React Fast Refresh handles the update. The SSR branch manually invalidates changed modules viainvalidateModule(mod, new Set(), timestamp, true)(matching Vite's own call) andreturn []. The explicit browserws.send({ type: "full-reload" })calls were removed.- Route add/delete full-reload correctly relocated to the
add/unlinkwatchers, gated on!hasAppDir(index.ts:4315,4351); hybrid usesinvalidateHybridClientEntries().routeChangedfires only for files matchingpageExtensionsunderpagesDiror app-route files. Correct. - New unit tests (
does not force full reload..., hybrid shared-code) and the E2Ehmr.spec.ts(state preservation, syntax-error recovery, MDX route updates) all pass.
Production parity for MDX — verified
The serve-only include: /\.(?:[tj]sx?|mdx)$/i is what enables MDX Fast Refresh in dev. I confirmed against main that the react plugin was previously invoked as default(reactOptions) with no .mdx in include for either command — so compiled MDX JSX never went through react's babel in build before this PR either. The build set here keeps the configured/default include (no .mdx), so production MDX parsing is unchanged. The limitToCommand apply guards are correct: refresh-wrapper (apply: "serve") is excluded from the build set, undefined-apply plugins run in both.
Transformed dev CSS collector — verified, non-blocking follow-ups
collectTransformedStylesheetAssets (dev-server.ts:154) walks the client module graph to discover transform-injected/virtual CSS, mirroring prod's SSR-manifest discovery. Dev/prod first-paint parity holds. Same non-blocking follow-ups noted in prior rounds still stand:
- Possible duplicate
<link>when the same CSS surfaces under differing URL forms (manifestcreatePagesDevAssetUrlvs rawimportedModule.url//@id/__x00__...);html.includes(href=...)only dedups identical hrefs. Dev-only, cosmetic. - CSS discovery only recurses into
type: "js"importers (dev-server.ts:195) — other/undefined types transitively importing CSS are skipped. Unlikely for Pages Router. - Per-request recursive graph walk with the cache cleared on every
add/change/unlinkrather than relevant edits only.seenModulesguards recursion. Dev-only. - E2E mutates committed fixtures (restored in
finally) + fixedpage.waitForTimeout(1_000)for syntax-error recovery — consider polling the overlay if it flakes.
Verification
vp check packages/vinext/src/index.ts packages/vinext/src/server/dev-server.ts tests/pages-router.test.ts— clean.Plugin config(15) — pass, including the previously-failingvinext:mdxtest.full reloadHMR unit tests (2) — pass.virtual CSStransform test — pass. All 9stylesheettests — pass.reacttests incl. double-registration guard (2) — pass.tests/next-config.test.ts(212) — pass, so thepageExtensions: [...,"mdx"]fixture addition doesn't perturb config/route parsing.
Dev-only change; prod-server.ts / worker-entry.ts untouched. LGTM pending CI (full Vitest + Playwright).
|
Review posted to PR #2544. SummaryI reviewed both commits of PR #2544 ( Key finding: the prior-round blocker is resolvedThe last review round found a real, reproducible Vitest failure — the pre-existing Verified correct
Non-blocking follow-ups (dev-only)Possible duplicate All targeted tests ( |
2 participants
Summary
Motivation
DigitecGalaxus/next-yak#569 added vinext to its bundler compatibility suite and exposed two dev-only gaps:
The HMR behavior now matches Next.js Pages Router Fast Refresh. Dev stylesheet discovery now follows Vite's transformed client module graph and emits blocking stylesheet links for generated CSS, matching production's first-paint behavior.
The automatic React integration now uses command-scoped plugin instances: production keeps the existing configured include behavior, while development adds
.mdxonly when the user did not explicitly configurereact.include. The MDX delegate is split into an early transform proxy and a later config proxy so compiled JSX reaches React Fast Refresh while the delegate is still initialized duringvinext:config.Validation
vp check packages/vinext/src/index.ts packages/vinext/src/server/dev-server.ts tests/pages-router.test.ts tests/fixtures/pages-basic/pages/hmr-state.tsx tests/e2e/pages-router/hmr.spec.tsintegration 4/10) — 398/398 pass locallyvinext-pagesdev matrix — 27/27 pass, including all seven HMR cases andpseudo-elementsRelated: DigitecGalaxus/next-yak#569