fix(pages): preload initial dev stylesheets#2423
Conversation
|
/bigbonk review for issues |
commit: |
|
![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 initial dev stylesheets
Reviewed the diff, ran the targeted tests (tests/pages-router.test.ts, tests/scss.test.ts), and vp check on all changed source + test files. Everything passes and the core approach is sound: emit blocking <link rel="stylesheet"> tags for the _app + matched-page CSS graph in dev (fixing FOUC), and align prod asset ordering (_app before page) with Next.js document ordering.
No correctness blockers found. A few observations below, mostly maintainability.
Non-blocking observations
1. Duplicated identical regexes (packages/vinext/src/index.ts:374-375)
const STYLESHEET_IMPORT_RE = /\.(?:css|scss|sass|less)$/i;
const STYLESHEET_FILE_RE = /\.(?:css|scss|sass|less)$/i;These two constants are byte-for-byte identical. Consider collapsing to one to avoid them drifting apart.
2. isPagesAppFile / isPotentialPagesAssetGraphScript defined three times
The same two helpers are defined inline in hotUpdate (index.ts:3598-3620) and again in configureServer (index.ts:3796-3819), and the assets-module invalidation loop is repeated across hotUpdate, the add/change/unlink watcher handlers. Worth hoisting to a single module-scope helper to keep the invalidation rules in one place — easy for these to diverge in a future edit.
3. Possible double full-reload for _app-graph script edits
For a script file in the _app graph, hotUpdate invalidates the assets module and sends { type: "full-reload" } (returning []), while watcher.on("change") also calls invalidatePagesClientAssetsModule(true) which sends another full-reload. Functionally harmless (idempotent reload) but fires the WS message twice per change.
4. Broad invalidation scope
isPotentialPagesAssetGraphScript matches any non-node_modules, non-app/ script under root. In a Pages app this means editing any unrelated .ts/.tsx (e.g. a util in lib/) invalidates the assets module and triggers a full reload. This is intentional to catch transitive _app deps in lib/, and pure Pages apps already full-reload on most script edits, so it's acceptable — just noting the widened scope.
Verified
- basePath handling:
createPagesDevAssetUrlemits root-relative hrefs (/styles/...); the final HTML passes throughserver.transformIndexHtml(dev-server.ts:2085) which applies Vite'sbase, so basePath (/docs/...) is correctly prepended. TheassetPrefix-should-not-leak test confirms this. - Query/type-only imports correctly excluded (
ignored.css?raw,type-only.module.css). - Manifest key lookup is robust via
getManifestFilesForModule's suffix-match fallback even though index.ts keys arenormalizePathSeparators'd while dev-server looks up rawroute.filePath/appAssetPath. - Prod ordering swap (
_appfirst) dedupes correctly incollectAssetTagsand all existing asset tests still pass.
|
Review posted to PR #2423. SummaryI reviewed PR #2423 ( Verdict: No correctness blockers. The approach is sound — emitting blocking Non-blocking observations posted:
Verified working:
|
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): preload initial dev stylesheets
Re-reviewed with a focus on correctness. The core approach is sound (emit blocking <link rel="stylesheet"> for the _app + matched-page CSS graph in dev, align prod _app-before-page ordering), and the test coverage for basePath/assetPrefix, transitive deps, type-only/query-only exclusion, HMR re-collection, and dev/prod parity is excellent.
I found one issue worth addressing before merge.
Possible blocker — App Router HMR degraded in hybrid (app + pages) apps
packages/vinext/src/index.ts:3596-3636
The hotUpdate guard changed from if (!hasPagesDir || hasAppDir) return; to if (!hasPagesDir) return;. Previously, in any app that has an app/ directory, this hotUpdate hook was a complete no-op and deferred entirely to the RSC plugin's fast-refresh. Now it runs for hybrid apps too.
The problem is isPotentialPagesAssetGraphScript (index.ts:3606-3620): it returns true for any script file under root that is not in node_modules and not literally inside app/. In a hybrid app, shared code like lib/*.ts, components/*.tsx, utils/*.ts is routinely imported by App Router routes. Editing such a file now hits:
if (pagesAppChanged || isPotentialPagesAssetGraphScript(options.file)) {
options.server.ws.send({ type: "full-reload" });
return []; // <-- runs BEFORE the `if (hasAppDir) return;` below
}Returning [] filters all modules out of the HMR update for downstream plugins, and the explicit full-reload forces a hard page reload. So in a hybrid app, editing a shared util that an App Router page imports now triggers a full page reload instead of React fast-refresh — a regression from main, where the same edit was handled by the RSC plugin's HMR.
The appDir exclusion only excludes files literally under app/; it does not exclude shared code that App Router consumes. The STYLESHEET_FILE_RE branch above is less severe (it only invalidates the assets module without forcing a reload before hasAppDir return), but the script branch returns early.
Suggested fix: gate the script-graph full-reload behind "pages-only" detection, or restrict the broad isPotentialPagesAssetGraphScript reload to apps without an app/ dir. For hybrid apps, scope the asset-module invalidation to actual transitive _app deps (or just invalidate without forcing full-reload/return []) so App Router fast-refresh is preserved. A hybrid HMR test would lock this down — the new tests are all pure-Pages.
Non-blocking
-
Duplicate identical regexes (
index.ts:374-375):STYLESHEET_IMPORT_REandSTYLESHEET_FILE_REare byte-for-byte identical — collapse to one to avoid drift. -
Helpers defined three times —
isPagesAppFile/isPotentialPagesAssetGraphScriptare defined inline inhotUpdate(index.ts:3598-3620) and again inconfigureServer(index.ts:3792-3820), with the same invalidation predicate repeated across theadd/change/unlinkwatchers. Hoist to module scope so the (now subtle) reload rules live in one place. -
Double
full-reload— for an_app-graph script edit,hotUpdatesendsfull-reloadandwatcher.on("change")callsinvalidatePagesClientAssetsModule(true)which sends another. Idempotent but redundant. -
Import attributes skipped (
index.ts:469):if (statement.attributes && statement.attributes.length > 0) continue;will skipimport "./x.css" with { type: "css" }. Edge case, likely fine, just noting.
|
Review posted to PR #2423. SummaryI re-reviewed PR #2423 ( Possible blocker — App Router HMR degraded in hybrid apps ( The options.server.ws.send({ type: "full-reload" });
return []; // runs BEFORE the `if (hasAppDir) return;` belowforcing a full page reload instead of React fast-refresh — a regression from Suggested fix: scope the script-graph full-reload to pages-only apps (or apps without an Non-blocking: duplicate identical regexes (374-375), the same helpers defined three times (hoist to module scope), a redundant double |
Performance benchmarksCompared 0 improved · 0 regressed · 6 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 |
There was a problem hiding this comment.
Review: fix(pages): preload initial dev stylesheets
Re-reviewed after the new ed8885a ("harden dev stylesheet parity") commit. That commit improves several things — ISR cached HTML and custom error pages now get the initial stylesheet head, error-page _app/error-page asset paths are resolved correctly, and .less was dropped from the stylesheet regexes. Good cleanups.
However, the new commit introduces a new correctness regression in addition to leaving a previously-flagged blocker unaddressed.
Blocker 1 (new) — ?raw / ?url / ?inline CSS imports are broken app-wide
packages/vinext/src/index.ts:2970-2973 (resolveId) and :476-481 / :400-402
The commit removed the query/hash guard from isStylesheetSpecifier:
// before (commit 7538189 — correct):
function isStylesheetSpecifier(specifier: string): boolean {
if (specifier.includes("?") || specifier.includes("#")) return false;
return STYLESHEET_IMPORT_RE.test(specifier.toLowerCase());
}
// after (commit ed8885a — strips the query before testing):
function isStylesheetSpecifier(specifier: string): boolean {
return STYLESHEET_IMPORT_RE.test(stripViteModuleQuery(specifier).toLowerCase());
}and added a new resolveId branch:
const cleanStylesheetId = stripViteModuleQuery(cleanId);
if (cleanStylesheetId !== cleanId && isStylesheetSpecifier(cleanId)) {
return await this.resolve(cleanStylesheetId, importer, { skipSelf: true });
}?raw, ?url, and ?inline are standard Vite import query modifiers with distinct, non-stylesheet semantics:
import s from "./a.css?raw"→ the CSS source as a stringimport u from "./a.css?url"→ the resolved URL stringimport "./a.css?inline"→ inlined, not injected as a<link>
This resolveId branch strips the query and re-resolves a.css?raw to the bare a.css CSS module, so the imported value is now the CSS-module/side-effect form instead of the raw string/URL — ?raw/?url/?inline for CSS are broken. And because this hook is not gated by command, it runs for both dev and production builds and for both App Router and Pages Router (filter \.(?:css|scss|sass)[?#]), so this is app-wide, not Pages-dev-only.
Separately, in collectDevPagesAppStylesheetAssets (:476), isStylesheetSpecifier("./a.css?raw") now returns true, so a ?raw import is added to the dev stylesheet manifest and emitted as a blocking <link rel="stylesheet">.
This is a deliberate behavior reversal between the two commits: in 7538189 the test asserted expect(html).not.toContain("ignored.css") for an import "@/styles/ignored.css?raw" (correctly excluded); in ed8885a that fixture was renamed to query.css?raw / hash.css#hash and the test now expects them emitted as stylesheets (devStylesheetHrefs contains /styles/query.css, /styles/hash.css). The first commit had the correct behavior.
Suggested fix: restore the query/hash exclusion for stylesheet detection (a CSS import carrying ?raw/?url/?inline/#… is not an initial stylesheet), and drop the resolveId query-stripping branch — let Vite resolve *.css?raw etc. with its normal semantics. The ?used/internal Vite CSS query is the only case that needs collapsing, and Vite already handles that; vinext shouldn't intercept user ?raw/?url/?inline. Add a regression test that a ?raw/?url/?inline CSS import is NOT emitted as <link> and still imports the raw/url/inline value.
Blocker 2 (still unaddressed) — App Router HMR degraded in hybrid (app + pages) apps
packages/vinext/src/index.ts:3601 and :3636-3640
This was flagged in the prior review and is unchanged. The hotUpdate guard is still if (!hasPagesDir) return;, and the script-graph branch still runs before if (hasAppDir) return;:
if (pagesAppChanged || isPotentialPagesAssetGraphScript(options.file)) {
options.server.ws.send({ type: "full-reload" });
return []; // line 3638
}
if (hasAppDir) return; // line 3640 — too lateisPotentialPagesAssetGraphScript returns true for any script under root that is not in node_modules and not literally inside app/. In a hybrid app, shared code (lib/*.ts, components/*.tsx, utils/*.ts) imported by App Router routes hits this branch, forcing a hard full-reload instead of React fast-refresh — a regression from main, where this hook was a no-op whenever hasAppDir. The appDir exclusion only excludes files literally under app/, not shared code App Router consumes. All new tests are pure-Pages, so this path stays uncovered.
Suggested fix: scope the script-graph full-reload/return [] to apps without an app/ dir (or to actual transitive _app deps). For hybrid apps, invalidate the assets module without forcing full-reload/return [] so App Router fast-refresh is preserved. Add a hybrid HMR test.
Non-blocking
-
Duplicate identical regexes (
index.ts:374-375):STYLESHEET_IMPORT_REandSTYLESHEET_FILE_REare still byte-for-byte identical (/\.(?:css|scss|sass)$/i). Collapse to one to avoid drift. -
Helpers defined three times —
isPagesAppFile/isPotentialPagesAssetGraphScriptare still defined inline inhotUpdateand again inconfigureServer, with the same invalidation predicate repeated across theadd/change/unlinkwatchers. Hoist to module scope so the reload rules live in one place. -
Double
full-reload— for an_app-graph script edit,hotUpdatesendsfull-reloadandwatcher.on("change")also callsinvalidatePagesClientAssetsModule(true)which sends another. Idempotent but redundant. -
import "./x.css" with { type: "css" }— skipped by thestatement.attributes.length > 0guard (index.ts:468). Edge case, likely fine, just noting.
Verified working
- ISR cached HTML and custom error pages now carry the initial stylesheet head (
dev-server.tscollectDevInitialStylesheetHeadHTML); error-page_app/error asset paths resolved viafindFileWithExts. - basePath handling via
transformIndexHtmlapplying Vite'sbase(assetPrefix correctly does not leak). - Type-only imports excluded; manifest key lookup robust via suffix-match fallback.
- Prod
_app-before-page ordering swap dedupes correctly; existing asset tests pass. - Targeted dev tests pass (which is exactly what codifies the Blocker 1 regression).
|
Review posted to PR #2423. SummaryI re-reviewed PR #2423 after the new Blocker 1 (new this commit) — Blocker 2 (still unaddressed) — App Router HMR degraded in hybrid apps. Non-blocking: duplicate identical regexes (374-375), helpers defined three times, redundant double Both blockers come with suggested fixes and recommended regression tests. |
|
/bigbonk review for issues |
There was a problem hiding this comment.
Review: fix(pages): preload initial dev stylesheets
Re-reviewed after the two new commits (3781e8f "preserve hybrid app HMR" and 14ca908 "preserve CSS resource query imports"). Both blockers from my prior review are now fixed, with proper regression tests. I ran the targeted tests and vp check on all changed files — everything passes. No remaining correctness blockers.
Blocker 1 (CSS ?raw/?url/?inline broken app-wide) — RESOLVED in 14ca908
The query/hash guard is restored in isStylesheetSpecifier (index.ts:400-402):
function isStylesheetSpecifier(specifier: string): boolean {
if (specifier.includes("?") || specifier.includes("#")) return false;
return STYLESHEET_IMPORT_RE.test(specifier.toLowerCase());
}and the resolveId query-stripping branch was removed, along with the \.(?:css|scss|sass)[?#] entry from the hook filter — so Vite now resolves *.css?raw/?url/?inline with its normal semantics. The test was corrected to the right behavior: query.css?raw is now asserted not to be emitted as a stylesheet (devStylesheetHrefs no longer contains /styles/query.css, SSR manifest not.toContain("styles/query.css"), and the prod test asserts the ?raw CSS marker is not bundled). This is exactly the fix I suggested.
Blocker 2 (App Router HMR degraded in hybrid apps) — RESOLVED in 3781e8f
The script-graph full-reload/return [] branch is now gated so it does not fire for hybrid apps (index.ts:3631):
if (pagesAppChanged || (!hasAppDir && pagesAssetGraphScriptChanged)) {
options.server.ws.send({ type: "full-reload" });
return [];
}
if (hasAppDir) return;Editing shared lib//components/ code that App Router routes import no longer forces a hard reload in hybrid apps — it falls through to if (hasAppDir) return; and defers to RSC fast-refresh. The asset-module invalidation (no forced reload) still runs so Pages stylesheet metadata stays fresh. The same !hasAppDir && pagesAssetGraphScriptChanged gating was applied consistently to the add/change/unlink watchers. A focused hybrid HMR test was added ("does not force full reload for shared App Router code in hybrid apps") asserting hotUpdateResult is undefined and no full-reload is sent — it passes.
Verified working
- Targeted dev/prod stylesheet tests pass, including the corrected
?raw/query exclusion and the new hybrid HMR test. - ISR cached HTML and custom error pages carry the initial stylesheet head;
_app/error asset paths resolved viafindFileWithExts. - basePath handling via
transformIndexHtmlapplying Vite'sbase(assetPrefix correctly does not leak); hrefs escaped viaescapeHtmlAttr, nonce applied. _app-before-page asset ordering preserved ([appAssetPath, route.filePath]).vp checkclean on all 6 changed files.
Non-blocking (carried over, low priority)
- Duplicate identical regexes (
index.ts:374-375):STYLESHEET_IMPORT_REandSTYLESHEET_FILE_REare still byte-for-byte identical (/\.(?:css|scss|sass)$/i). Collapse to one to avoid drift. - Helpers defined twice —
isPagesAppFile/isPotentialPagesAssetGraphScriptare defined inline inhotUpdateand again inconfigureServer. Hoist to module scope so the (now subtle) reload rules live in one place; this is also what would have made the two-commit fix a one-line change. import "./x.css" with { type: "css" }— skipped by thestatement.attributes.length > 0guard (index.ts:469). Edge case, likely fine, just noting.
Nice work addressing both blockers cleanly. LGTM once you optionally fold in the non-blocking cleanups.
|
Review posted to PR #2423. SummaryI re-reviewed PR #2423 after the two newest commits ( Blocker 1 (CSS
Blocker 2 (App Router HMR degraded in hybrid apps) — RESOLVED in
Non-blocking (carried over): duplicate identical regexes (374-375), the two helpers defined twice (hoist to module scope), and Clean fixes for both blockers — LGTM. |
1 participant
Summary
Validation