fix(dev): adopt initial Pages stylesheets in Vite - #2825
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 |
dcb8526 to
343258f
Compare
343258f to
ecd9ec0
Compare
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
@james-elicx Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review for issues |
Review:
|
|
Posted the review to PR #2825. Summary of findings: Verdict: Approach is correct. Verified against the actual Vite dev client ( Issues raised (none blocking):
Minor: unreachable else-branch at line 215; |
|
/bigbonk review for issues |
Re-review:
|
|
Posted the review to PR #2825. SummaryRe-reviewed after commit Resolved since last review:
Main remaining issue (non-blocking, worth nailing down before merge):
Minor: likely-unreachable else-branch at Verdict: approach is correct and the adoption contract holds; the |
|
/bigbonk review for issues |
|
UnknownError: ProviderInitError |
|
/bigbonk review for issues |
There was a problem hiding this comment.
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 assetloadhandler actually tests before serving a module as an inertexport default "<source>"string (node.js:26466, filter at:26458).rawRE = /[?&]raw\b/—node.js:30114. This is used only by the loading-access checkcheckLoadingAccess(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 olddev-server.tsblock that got deleted). The surviving branch iscreateViteStylesheetHrefat: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
940d47dvia the sharedappendStylesheetAsset+stylesheetAssetKeys(emitting bothid:andurl: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 passedvp test run tests/pages-router.test.ts -t "stylesheet"— 9 passedvp checkon both touched source files — cleanvp 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.
|
Review posted to PR #2825. SummaryI did an independent re-review at Key findingThe earlier review cited the wrong regex. Vite has two
The PR's Other items
Verification
Verdict: LGTM. Only optional nit: name the loader regexes in the |
Summary
Regression coverage
main, the repro emits three links for two imports and injects two additional styles after hydrationValidation
vp check