Skip to content

fix(dev): adopt initial Pages stylesheets in Vite - #2825

Merged
james-elicx merged 3 commits into
mainfrom
codex/fix-pages-dev-css-adoption
Aug 7, 2026
Merged

fix(dev): adopt initial Pages stylesheets in Vite#2825
james-elicx merged 3 commits into
mainfrom
codex/fix-pages-dev-css-adoption

Conversation

@james-elicx

Copy link
Copy Markdown
Member

Summary

  • mark server-rendered Pages dev stylesheet links with the exact module IDs used by Vite
  • let the Vite client adopt initial links instead of injecting duplicate style elements after hydration
  • centralize stylesheet identity, deduplication, and URL handling in a focused helper while preserving CSP nonces

Regression coverage

  • adds a real Pages route with two CSS imports, middleware-provided CSP nonce, and cascade-sensitive rules
  • live browser coverage asserts exactly two initial nonced links, no duplicate injected Vite styles, and stable cascade
  • on unmodified main, the repro emits three links for two imports and injects two additional styles after hydration

Validation

  • focused stylesheet helper and Pages integration tests
  • live Playwright regression
  • vinext build and touched-file vp check
  • independent Vite client-contract review: clean

@pkg-pr-new

pkg-pr-new Bot commented Aug 6, 2026

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/@vinext/cloudflare@2825
npm i https://pkg.pr.new/create-vinext-app@2825
npm i https://pkg.pr.new/@vinext/types@2825
npm i https://pkg.pr.new/vinext@2825

commit: 1d5190d

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor
Example Preview Production Original
app-router-cloudflare preview production
pages-router-cloudflare preview production
app-router-playground preview production original
realworld-api-rest preview production
nextra-docs-template preview production
benchmarks preview production
hackernews preview production original
workers-cache preview production
web preview production

@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Performance benchmarks

Compared 1d5190d against base 3c5cea4 using alternating same-runner rounds. Next.js was unchanged and skipped.

0 improved · 0 regressed · 6 within ±1.5%

Scenario Framework Baseline Current Change
Client bundle size (gzip) vinext 134.6 KB 134.7 KB ⚫ +0.0%
Client entry size (gzip) vinext 122.2 KB 122.2 KB ⚫ +0.0%
Dev server cold start vinext 2.21 s 2.21 s ⚫ -0.1%
Production build time vinext 2.39 s 2.38 s ⚫ -0.1%
RSC entry closure size (gzip) vinext 113.8 KB 113.9 KB ⚫ +0.0%
Server bundle size (gzip) vinext 191.4 KB 191.4 KB ⚫ +0.0%

View detailed results and traces

🟢 improvement · 🔴 regression · ⚫ change below 1.5% · paired base/head

@james-elicx
james-elicx force-pushed the codex/fix-pages-dev-css-adoption branch from dcb8526 to 343258f Compare August 6, 2026 10:00
@james-elicx
james-elicx force-pushed the codex/fix-pages-dev-css-adoption branch from 343258f to ecd9ec0 Compare August 6, 2026 10:22
@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

UnknownError: ProviderInitError

github run

@ask-bonk

ask-bonk Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

@james-elicx Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review: fix(dev): adopt initial Pages stylesheets in Vite

Reviewed the CSS-adoption refactor against the Vite client contract. The core approach is sound and correctly matches how the Vite dev client dedups stylesheets. Verified against the bundled client (@voidzero-dev/vite-plus-core client.mjs):

  • linkSheetsMap is populated at module init from link[rel="stylesheet"][data-vite-dev-id] (client.mjs:1185).
  • updateStyle(id) returns early when linkSheetsMap.has(id) (client.mjs:1191).

So server-rendering the link with the exact data-vite-dev-id that Vite passes to updateStyle() is what suppresses the duplicate <style> injection. resolvePagesDevStylesheetId correctly reproduces that id (resolved transform id incl. query, \0 ids rejected as non-adoptable). Nonce threading, cascade order, and the encoded-filename fixture all check out. Focused + integration tests pass locally.

A few issues, none blocking:

1. NON_EXECUTING_MODULE_QUERY_RE diverges from Vite's actual special-query semantics

pages-dev-stylesheets.ts:12 uses (?:&|$) to terminate, so component.js?raw=false is treated as a non-boundary (executable) and gets traversed as JS. But Vite's own regexes use \b:

rawRE = /[?&]raw\b/;    urlRE = /[?&]url\b/;    inlineRE = /[?&]inline\b/;    directRequestRE = /[?&]direct\b/;

(from @voidzero-dev/vite-plus-core node.js — the =false value is ignored; presence of the key is what matters). The stylesheet regex on line 13 correctly uses \b and matches Vite; the module regex on line 12 does not. In practice a ?raw/?url JS module exports a string and has no traversable CSS imports, so the divergence is likely benign — but it is an intentional-looking asymmetry (encoded in the tests at pages-dev-stylesheets.test.ts:54-62) that contradicts Vite's documented behavior. Worth a comment explaining why the two boundary rules differ, or aligning both to \b.

2. Duplicated stylesheet-key derivation

The dedup key is computed inline at pages-dev-stylesheets.ts:147 and again in stylesheetAssetKey (line 195-197) with identical logic. These must stay in sync or the intra-collection dedup and the cross-source merge (line 209) will key differently. Recommend having addStylesheet call stylesheetAssetKey({ href, viteDevId }) so there is a single source of truth.

3. Cross-source dedup gap when viteDevId resolution disagrees

collectPagesDevInitialStylesheetHeadHTML merges manifest and transformed assets by stylesheetAssetKey. Manifest assets key on url: when resolveUrl fails/returns a \0 id, while the transformed pass may resolve the same file to a non-null id and key on id:. In that case the same CSS file would emit two <link>s. The current fixtures always resolve to matching ids so this never fires, but it is a latent duplicate-link path. Consider a fallback that also dedups a null-id manifest asset against a resolved-id transformed asset for the same underlying file.

Minor

  • pages-dev-stylesheets.ts:215 — the rawHref.startsWith("/") ? ... : createPagesDevAssetUrl(rawHref) else-branch appears unreachable; every asset href at this point already starts with /.
  • \0-virtual CSS modules still emit a link with no data-vite-dev-id (line 139-141), so Vite will inject a duplicate <style> for them on hydration. This is the documented adoptableViteDevId trade-off — fine to leave, but worth an inline note where the /@id/__x00__... href is built.

Nice, well-commented helper overall and good regression coverage (space-in-filename encoding, cascade order, CSP nonce, two-imports-two-links).

@ask-bonk

ask-bonk Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Posted the review to PR #2825. Summary of findings:

Verdict: Approach is correct. Verified against the actual Vite dev client (client.mjs) that the data-vite-dev-id adoption contract works exactly as the PR describes — server-rendered links populate linkSheetsMap, and updateStyle short-circuits, suppressing duplicate <style> injection. Nonce, cascade order, and encoded-filename handling all check out. Focused + integration tests pass locally.

Issues raised (none blocking):

  1. Boundary-regex asymmetry vs Vite semanticsNON_EXECUTING_MODULE_QUERY_RE (line 12) uses (?:&|$) while Vite's real rawRE/urlRE/inlineRE/directRequestRE all use \b. So component.js?raw=false is traversed as executable JS, contradicting Vite's behavior. Likely benign (raw JS has no CSS imports), but the intentional-looking asymmetry deserves a comment or alignment.

  2. Duplicated key derivation — the dedup key is computed inline at line 147 and again in stylesheetAssetKey (line 195). Should share one function to avoid drift.

  3. Latent cross-source dedup gap — if manifest resolution yields a null id (url: key) but the transformed pass resolves the same file to a real id (id: key), the same CSS could emit two links. Fixtures never trigger it, but it's a latent path.

Minor: unreachable else-branch at line 215; \0-virtual CSS still double-injects (documented trade-off).

github run

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Re-review: fix(dev): adopt initial Pages stylesheets in Vite

Re-reviewed after commit 940d47d, which addresses the two substantive issues from the previous round:

  • Duplicated key derivation (prev Published npm package ships sourcemaps pointing to missing source files #2) — now resolved. appendStylesheetAsset + stylesheetAssetKeys are the single source of truth; both collectTransformedStylesheetAssets and collectPagesDevInitialStylesheetHeadHTML call the shared merge (pages-dev-stylesheets.ts:150, :240).
  • Cross-source dedup gap (prev fix: exclude vinext from optimizeDeps to prevent virtual module resolution errors #3) — now resolved. stylesheetAssetKeys emits both id: and url: keys, so a null-id manifest asset dedups against a resolved-id transformed asset for the same file, and the merge prefers the adoptable (id-bearing) asset (:217). Covered by the new "deduplicates a manifest fallback against the later resolved graph asset" test (pages-dev-stylesheets.test.ts:149).

The core adoption contract (server-render the link with the exact data-vite-dev-id Vite passes to updateStyle(), so the client short-circuits duplicate <style> injection) still holds. Focused + integration tests pass locally.

1. ?raw=false on non-CSS modules still diverges from Vite — and now I can show it's not obviously benign

Verified against the bundled @voidzero-dev/vite-plus-core:

const urlRE  = /[?&]url\b/;   // node.js:30113
const rawRE  = /[?&]raw\b/;   // node.js:30114

rawRE matches ?raw=false (the \b sits between w and =). Vite's asset plugin then serves that module as a raw string with no executable body:

if (rawRE$1.test(id)) {
  const file = checkPublicFile(id, config) || cleanUrl(id);
  return { code: `export default ${JSON.stringify(await fsp.readFile(file, "utf-8"))}`, ... }; // node.js:26466
}

The PR's NON_EXECUTING_MODULE_QUERY_RE = /[?&](?:raw|url|worker|sharedworker)(?:&|$)/ (pages-dev-stylesheets.ts:14) uses (?:&|$) instead of \b, so it does not match ?raw=false. The result: component.js?raw=false is classified as executable JS and traversed (:170), and its CSS import is emitted as an initial <link>.

The stylesheet regex on the next line already uses \b (NON_INJECTING_STYLESHEET_QUERY_RE, :15), so style.css?raw=false is correctly treated as a boundary. The divergence is isolated to the module regex on line 14, and it's baked into the tests as intended behavior (pages-dev-stylesheets.test.ts:54-62 and the "collects CSS imported through an executable JavaScript query wrapper" case at :98, which asserts raw-value-wrapper.js?raw=false's CSS is collected).

Since Vite serves ?raw modules as inert strings, a real .js?raw=false import should not contribute an injected stylesheet at runtime — so emitting an initial <link> for it risks an extra, never-adopted stylesheet, not a benign no-op. The focused test passes only because it drives the collector directly rather than asserting what Vite's client actually injects at hydration. Recommend one of:

  • align line 14 to \b (matching Vite and the sibling regex on line 15), and update the tests to assert ?raw=false JS is a boundary; or
  • if the (?:&|$) behavior is deliberate, add a comment explaining why it must diverge from Vite's rawRE/urlRE, and add an assertion that the emitted link corresponds to a stylesheet the Vite client genuinely injects (otherwise the test is asserting an over-emission).

The existing comment on lines 9-13 explains why ?raw=false "still executes," but that reasoning contradicts the bundled asset plugin above — worth reconciling.

Minor

  • pages-dev-stylesheets.ts:245rawHref.startsWith("/") ? rawHref : createPagesDevAssetUrl(rawHref). Every asset reaching this point already has a /-prefixed href (createPagesDevAssetUrl on the manifest side, moduleNode.url / /@id/... on the transformed side), so the else-branch looks unreachable. Either drop it or leave a note on the invariant.
  • \0-virtual CSS modules emit /@id/__x00__...?direct with no data-vite-dev-id (:142-143), so Vite will inject a duplicate <style> for them at hydration. This is the documented adoptableViteDevId trade-off; fine to keep, but an inline note at the /@id/__x00__ href construction would flag it for future readers.

Good regression coverage overall — space-in-filename encoding, cascade order, CSP nonce threading, and the two-imports-two-links live browser assertion are all solid. The ?raw=false classification is the one thing I'd nail down before merge.

@ask-bonk

ask-bonk Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Posted the review to PR #2825.

Summary

Re-reviewed after commit 940d47d. Two of the prior review's issues are now fixed; one substantive concern remains.

Resolved since last review:

  • Duplicated key derivationappendStylesheetAsset + stylesheetAssetKeys are now the single source of truth, used by both collection paths.
  • Cross-source dedup gapstylesheetAssetKeys emits both id: and url: keys, so a null-id manifest asset dedups against a resolved-id transformed asset for the same file (and the merge prefers the adoptable one). Covered by a new dedicated test.

Main remaining issue (non-blocking, worth nailing down before merge):

  • ?raw=false on non-CSS modules diverges from Vite. I verified against the bundled @voidzero-dev/vite-plus-core that rawRE = /[?&]raw\b/ matches ?raw=false and Vite serves such modules as inert export default "<source>" strings (asset plugin at node.js:26466). The PR's NON_EXECUTING_MODULE_QUERY_RE uses (?:&|$) instead of \b, so component.js?raw=false gets traversed as executable JS and its CSS is emitted as an initial <link>. Since Vite never executes that raw module, this risks an extra, never-adopted stylesheet — not the benign no-op the previous review assumed. The sibling stylesheet regex on the next line correctly uses \b, making the asymmetry look accidental. The focused test passes only because it drives the collector directly rather than asserting what Vite's client actually injects. Recommended aligning line 14 to \b (and updating tests) or documenting/justifying the divergence with a runtime-injection assertion.

Minor: likely-unreachable else-branch at :245; \0-virtual CSS still double-injects (documented trade-off, would benefit from an inline note).

Verdict: approach is correct and the adoption contract holds; the ?raw=false classification is the one thing I'd resolve before merge.

github run

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk

ask-bonk Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

UnknownError: ProviderInitError

github run

@james-elicx

Copy link
Copy Markdown
Member Author

/bigbonk review for issues

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review: fix(dev): adopt initial Pages stylesheets in Vite

Independent re-review at 1d5190d, focused on verifying the one issue prior rounds flagged as "the thing to resolve before merge." I checked it against the actual bundled @voidzero-dev/vite-plus-core (0.2.6), and the conclusion flips: the PR is correct and the ?raw=false concern is a false alarm.

The ?raw=false divergence is not a bug — the earlier review cited the wrong regex

There are two distinct raw regexes in the bundled Vite, and they mean different things:

  • rawRE$1 = /(\?|&)raw(?:&|$)/node.js:4106. This is what the asset load handler actually tests before serving a module as an inert export default "<source>" string (node.js:26466, filter at :26458).
  • rawRE = /[?&]raw\b/node.js:30114. This is used only by the loading-access check checkLoadingAccess (node.js:30118), never to decide whether the module is served raw.

The previous review quoted the \b variant (node.js:30114) and concluded ?raw=false is served raw. But the code path that decides "serve as inert string" uses the (?:&|$) variant, which does not match ?raw=false. Same story for url/worker/sharedworker:

  • urlRE$1 = /(\?|&)url(?:&|$)/ (node.js:4105)
  • workerOrSharedWorkerRE = /(?:\?|&)(worker|sharedworker)(?:&|$)/ (node.js:32517)

So NON_EXECUTING_MODULE_QUERY_RE = /[?&](?:raw|url|worker|sharedworker)(?:&|$)/ (pages-dev-stylesheets.ts:15) mirrors Vite's real loader matchers exactly, including the =false cases. Verified by direct comparison:

/c.js?raw          viteRaw$1: true   PR: true
/c.js?raw=false    viteRaw$1: false  PR: false
/c.js?worker       viteWorker: true  PR: true
/c.js?worker=false viteWorker: false PR: false

And the fixture in pages-dev-stylesheets.test.ts:129-147 demonstrates the ground truth: transformRequest("/raw-value-wrapper.js?raw=false") returns import "/raw-value-wrapper.css" (executable JS, CSS edge retained), while ?raw returns export default "..." with no imported modules. So ?raw=false genuinely executes and its CSS import is a real injected stylesheet — emitting an initial <link> for it is correct, not an over-emission.

The NON_INJECTING_STYLESHEET_QUERY_RE (:16) correctly uses \b, because the CSS-specific inline/direct matchers do use \b (inlineRE = /[?&]inline\b/ node.js:32519, directRequestRE = /[?&]direct\b/ node.js:27571). The asymmetry between line 15 and line 16 is intentional and matches Vite. The explanatory comment on :9-14 is accurate. I'd suggest only a tiny tweak: cite the loader regexes (rawRE$1/workerOrSharedWorkerRE) by name in that comment so a future reader doesn't repeat the \b-vs-(?:&|$) mix-up.

Core adoption contract holds

Confirmed against client.mjs: linkSheetsMap is populated from link[rel="stylesheet"][data-vite-dev-id] at init (client.mjs:1185-1186) and updateStyle(id) returns early when linkSheetsMap.has(id) (client.mjs:1190-1191). Server-rendering the link with the exact resolved id suppresses the duplicate <style>. \0 ids are correctly rejected as non-adoptable (:54-59).

Prior "minor" items — status

  • Unreachable else-branch: the rawHref.startsWith("/") ? … : createPagesDevAssetUrl(rawHref) branch the earlier review flagged no longer exists (it was in the old dev-server.ts block that got deleted). The surviving branch is createViteStylesheetHref at :189, and its non-/ path is exercised by the virtual-module test (:228-253, asserting /@id/virtual:safe-style.css?resolved&direct). Not unreachable, no action needed.
  • Key-derivation duplication / cross-source dedup gap: resolved in 940d47d via the shared appendStylesheetAsset + stylesheetAssetKeys (emitting both id: and url: keys, preferring the adoptable asset at :227), covered by the dedicated test at :175-210. Verified.
  • \0-virtual CSS double-injects at hydration: still true (:191-197), documented trade-off. An inline note at the /@id/__x00__ construction would help future readers, but it's fine to leave.

Verification performed

  • vp test run tests/pages-dev-stylesheets.test.ts — 7 passed
  • vp test run tests/pages-router.test.ts -t "stylesheet" — 9 passed
  • vp check on both touched source files — clean
  • vp run vinext#build — completes

Verdict: LGTM. Approach is correct, regression coverage is strong (space-in-filename encoding, cascade order, CSP nonce, two-imports-two-links live assertion, manifest→graph dedup), and the previously-flagged ?raw=false "blocker" is a false alarm rooted in citing the access-check regex instead of the asset loader regex. Only optional nit: reference the loader regexes by name in the :9-14 comment.

@ask-bonk

ask-bonk Bot commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Review posted to PR #2825.

Summary

I did an independent re-review at 1d5190d, focusing on the ?raw=false concern that prior review rounds flagged as the one thing to resolve before merge. Verifying against the actual bundled @voidzero-dev/vite-plus-core (0.2.6), the conclusion reverses: the PR is correct and the concern is a false alarm.

Key finding

The earlier review cited the wrong regex. Vite has two raw regexes with different meanings:

  • rawRE$1 = /(\?|&)raw(?:&|$)/ (node.js:4106) — used by the asset loader (:26466) to decide whether to serve a module as an inert export default "<source>" string. Does not match ?raw=false.
  • rawRE = /[?&]raw\b/ (node.js:30114) — used only for the loading-access check, never for load behavior. Matches ?raw=false.

The PR's NON_EXECUTING_MODULE_QUERY_RE mirrors the loader regexes (rawRE$1, urlRE$1, workerOrSharedWorkerRE — all (?:&|$)) exactly. I confirmed the ground truth in the PR's own fixture: transformRequest("...?raw=false") returns executable JS retaining the CSS import, while ?raw returns an inert string with no imports. So emitting an initial <link> for ?raw=false is correct, not an over-emission. The \b-vs-(?:&|$) asymmetry between lines 15 and 16 is intentional and matches Vite (CSS-specific inline/direct do use \b).

Other items

  • Core adoption contract verified against client.mjs (updateStyle short-circuits on linkSheetsMap.has(id)).
  • The "unreachable else-branch" from prior reviews no longer exists (deleted with old dev-server code); the surviving branch is exercised by the virtual-module test.
  • Key-derivation duplication and cross-source dedup gap were resolved in 940d47d, with a dedicated test.

Verification

pages-dev-stylesheets.test.ts (7 passed), pages-router.test.ts -t stylesheet (9 passed), vp check clean, build completes.

Verdict: LGTM. Only optional nit: name the loader regexes in the :9-14 comment so future readers don't repeat the regex mix-up.

github run

@james-elicx
james-elicx marked this pull request as ready for review August 7, 2026 10:49
@james-elicx
james-elicx merged commit 884259a into main Aug 7, 2026
66 checks passed
@james-elicx
james-elicx deleted the codex/fix-pages-dev-css-adoption branch August 7, 2026 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant