fix(css): preserve distinct media filenames for CSS url() assets#1725
Conversation
Match Next.js `asset/resource` behaviour for CSS `url()` dependencies: emit them as files under `/_next/static/media/` instead of inlining, and keep byte-identical sources (e.g. `dark.svg` / `dark2.svg`) as separate output files instead of letting Vite/Rolldown's content-based asset dedupe collapse them onto one filename. There is no config switch to disable that dedupe (vitejs/vite#8632). The native escape hatch is `this.emitFile({ fileName })`, which is never deduped. A build-only, client-scoped pair of plugins: - mark: during the client CSS transform, tag each relative asset `url()` with a private `?vinext_css_url_asset=<source-basename>` query — the only durable carrier of per-reference provenance, since once Rolldown dedupes, the bundle can't tell which reference came from which source. - restore: at `generateBundle`, read the marker back, emit a sibling file under the wanted source name via the `fileName` escape hatch, rewrite the reference, and strip the marker. Each reference resolves from its own marker, so split CSS chunks stay correct without a shared cursor or bundle-iteration-order dependence. This is a slimmer reimplementation of the approach explored in #1703: same mark/restore mechanism and public surface, but the restore side drops the AssetIndexes machinery, filename-reservation loop, and multi-fallback lookups in favour of a single basename map + emitted-set, since the sibling-filename construction (source-stem + content-hash + ext) makes those guarded cases unreachable. Tests port the Next.js scss/url-global e2e into the committed `pages-basic` and `app-basic` fixtures (built in isolation), covering the single-stylesheet two-identical-urls case and cross-chunk provenance. Co-authored-by: Nathan Nguyen <146415969+NathanDrake2406@users.noreply.github.com>
commit: |
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
/bigbonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — fix(css): preserve distinct media filenames for CSS url() assets
Verified locally on the PR head (d43c21b):
vp test run tests/css-url-assets.test.ts→ 8/8 passvp test run tests/build-optimization.test.ts→ 80/80 passvp checkon both changed source files → format/lint/type cleanmd5sumconfirmsdark.svg/dark2.svgare byte-identical in all four fixture copies — exactly the input that triggers Rolldown's content dedupe this PR repairs.
Overall this is a clean, well-scoped implementation. The mark/restore mechanism is the right approach given there's no config switch to disable Rolldown's content dedupe, and per-reference markers (vs. #1703's shared cursor / AssetIndexes) are genuinely simpler and correctly handle the split-chunk provenance case. Build-only + client-scoped is the correct boundary — dev needs no fix because Vite's dev middleware serves byte-identical sources at distinct URLs already, and RSC/SSR asset handling is untouched.
A few notes, none blocking.
Correctness / parity
-
Sibling files reuse the deduped asset's content hash.
deriveSiblingFileNameswaps only the stem (dark.HASH.svg→dark2.HASH.svg), so both files share one hash. This matches Next.js (webpack also content-hashes, so byte-identical sources get the same hash) — good. Worth noting explicitly since it means the two emitted files are byte-identical and hash-identical, differing only in stem. -
Global
assetsInlineLimit: 0affects all client assets, not just CSSurl()deps. The PR title frames this as CSSurl()parity, but the default also stops inlining JS-imported assets on the client. This is defensible (webpackasset/resourcedoesn't inline these either, and user config still wins), and the comment documents it — just flagging that the blast radius is broader than "CSS url()" alone. Confirm this is the intended parity choice.
Minor
-
transformreturnsmap: nullwith the comment "never shifts line/column positions". Appending?vinext_css_url_asset=…insideurl(...)does shift columns of subsequent tokens on that line. In practice this is harmless — the marker is stripped atgenerateBundle, so final output positions are restored, and CSS asset-url sourcemaps are low value — but the comment's justification is slightly inaccurate. Consider rewording to "marker is transient and stripped before final output" rather than claiming no position shift. -
assetsByBasekeys by output basename across the whole bundle. Two non-CSS assets in different dirs sharing a basename would collide. With content-hashed names this is effectively unreachable, and CSS files are correctly excluded — fine to leave, but a one-line comment noting the basename-uniqueness assumption would help future readers. -
CodeQL ReDoS finding on
client-build-config.ts:17(/\/+$/) is a false positive: the pattern is anchored, single-char, and linear;assetsDiris build-time config, not uncontrolled runtime input. Safe to dismiss. -
vinext:client-css-url-assets-defaults(configEnvironment) is partially redundant with the explicitenvironments.client.build.assetsInlineLimitassignments. It's a harmless belt-and-suspenders carrier for the default client env, and applies the same value, so no behavioral issue — just noting the duplication in case it's worth consolidating.
Test coverage
The split-chunk test (css-url-split-a/-b referencing the two identical svgs from separate page chunks) is the strongest addition — it's precisely the per-reference-provenance scenario where a shared cursor would break, and it's good that the per-marker design is exercised here. Unit coverage of inlined-chunk marker stripping (escaped quotes, query/fragment preservation) is thorough.
LGTM pending confirmation on point (2).
|
Review posted to PR #1725. SummaryI reviewed the CSS
Verdict: LGTM pending one confirmation. The mark/restore design is sound, the per-reference marker approach is genuinely simpler than #1703's shared-cursor scheme and correctly handles split-chunk provenance, and the build-only/client-scoped boundary is the right call (dev needs no fix; RSC/SSR untouched). Findings (none blocking):
|
…ify comments - tests/css-url-assets.test.ts: merge the two `vite` imports into one (import/no-duplicates lint error that failed CI Check). - client-build-config.ts: replace `/\/+$/` trailing-slash strip with a linear loop so CodeQL stops flagging a (false-positive) ReDoS. - index.ts: reword the css-url mark transform's `map: null` comment — the marker shifts columns transiently but is stripped before final output. - css-url-assets.ts: note the basename-uniqueness assumption in assetsByBase. Addresses review feedback on #1725. Co-authored-by: Nathan Nguyen <146415969+NathanDrake2406@users.noreply.github.com>
|
Thanks for the thorough review! Pushed CI failures
Review notes
|
The url() asset tests previously added routes to the shared app-basic /
pages-basic fixtures and built those whole apps in isolation. That made
the tests CPU-heavy and, run concurrently under CI, contended with
app-router.test.ts's static-export test enough to push it past its 30s
timeout (it passes comfortably locally and on main; the extra parallel
load was the trigger). The added app-basic route also slightly inflated
every app-basic-wide test (static export renders all routes).
Move the fixtures into dedicated minimal committed apps
(tests/fixtures/css-url-assets-{app,pages}) that build in ~1-2s. Same
observable contract (byte-identical dark.svg/dark2.svg referenced from
one stylesheet, plus the split-chunk provenance case), now isolated from
the heavyweight shared fixtures and fast enough not to contend.
Co-authored-by: Nathan Nguyen <146415969+NathanDrake2406@users.noreply.github.com>
|
CI is green now. Two follow-up commits since the review:
On the integration-shard failures: they were flaky 30s timeouts on the heaviest shard (
The dedicated-fixtures change still reduces the blast radius the right way: the tests no longer build the full |
|
/bigbonk review |
|
@james-elicx Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
|
/bigbonk review |
1 similar comment
|
/bigbonk review |
There was a problem hiding this comment.
Review — fix(css): preserve distinct media filenames for CSS url() assets (re-review on 10fdf10)
Re-reviewed after the two follow-up commits (d2bf995 lint/ReDoS/wording, 10fdf10 dedicated fixtures). Verified locally on the PR head:
vp test run tests/css-url-assets.test.ts→ 8/8 passvp test run tests/build-optimization.test.ts→ 80/80 passvp checkon all 4 changed source/test files → format/lint/type cleanmd5sumconfirms all fourdark.svg/dark2.svgfixture copies are byte-identical (ed5a143…) — the exact input that triggers Rolldown's content dedupe.
The previous review's findings are all addressed: map: null comment reworded, assetsByBase basename-collision comment added, ReDoS regex replaced with a linear loop, duplicate vite import merged. The assetsInlineLimit: 0 blast radius and shared-hash sibling behaviour were confirmed as intended Next-parity choices. The new build-optimization.test.ts case that drives createBuilder and asserts user build.assetsInlineLimit wins across all three environments is a strong addition.
The dedicated-fixtures refactor is a clear improvement — the tests no longer build the full app-basic/pages-basic in isolation, and the fixtures are committed (not written at test time) with good explanatory comments tying each file back to the upstream scss/url-global case.
LGTM. One non-blocking nit below.
Minor (non-blocking)
Stale header comment in tests/css-url-assets.test.ts (lines 9–20). The file's top comment still describes the pre-10fdf10 layout:
these exercise plain CSS against the committed
pages-basic/app-basicfixtures:
- pages-basic: styles/css-url-global.module.css …
- app-basic: app/nextjs-compat/css-test/url-assets …
Those paths no longer exist (tests/fixtures/pages-basic/styles/css-url-global.module.css and tests/fixtures/app-basic/app/nextjs-compat/css-test/url-assets are both gone). The tests now use the dedicated tests/fixtures/css-url-assets-{pages,app} fixtures, which the PAGES_CSS_FIXTURE/APP_CSS_FIXTURE constants and the in-body comment at lines 32–34 correctly describe. Worth updating the header block to match so future readers aren't sent looking for files that don't exist.
Notes verified during review
- All callers of the changed
getClientOutputConfigForVite/createClientOutputConfigsignatures (now takingassetsDir) are updated consistently acrossindex.ts(4 sites) andclient-build-config.ts. - Nested
css//media/asset subtrees serve correctly: the prod server maps/_next/static/...1:1 to disk, and the integration test fetches the emitted svg URLs and asserts200+image/svg+xml. - The marker is appended to existing query strings and stripped via
dropMarker/stripChunkMarkers, preserving other params and fragments (covered by the unit tests).decodeURIComponentSafeguards against a stray%aborting the rewrite. - Build-only + client-scoped boundary holds: the three plugins are all
apply: "build"and gated onthis.environment?.name === "client", so dev CSS and RSC/SSR asset handling stay untouched.
The flaky integration-shard timeouts called out in the PR thread are unrelated to this change (the plugins never load in dev, and the failing tests don't touch CSS assets).
|
Review posted to PR #1725. SummaryRe-reviewed the CSS
Verdict: LGTM with one non-blocking nit. The prior review's six findings are all addressed (map comment reworded, basename-collision comment added, ReDoS regex replaced with a linear loop, duplicate import merged, blast-radius/shared-hash confirmed as intended Next-parity). The dedicated-fixtures refactor is a clear improvement, and the new One nit found: The header comment block in I also independently verified: all callers of the changed config signatures are updated, nested |
…ixtures
The file header still described the pre-refactor layout (pages-basic /
app-basic paths that no longer exist). Point it at the dedicated
css-url-assets-{pages,app} fixtures. Comment-only.
Co-authored-by: Nathan Nguyen <146415969+NathanDrake2406@users.noreply.github.com>
|
Thanks for the re-review! Fixed the stale header comment in (The remaining inline CodeQL annotation on |
|
/bigbonk re-review |
1 similar comment
|
/bigbonk re-review |
There was a problem hiding this comment.
Re-review — fix(css): preserve distinct media filenames for CSS url() assets (fd409cb2)
Re-reviewed after the header-comment fix (fd409cb2, comment-only). Verified locally on the PR head:
vp test run tests/css-url-assets.test.ts→ 8/8 passvp test run tests/build-optimization.test.ts→ 80/80 passvp checkon all 4 changed source/test files → format/lint/type cleanmd5sumconfirms all fourdark.svg/dark2.svgfixture copies are byte-identical (ed5a143…) — the exact input that triggers Rolldown's content dedupe.
Verdict: LGTM. The last review's single non-blocking nit is resolved.
The stale header nit is fixed
fd409cb2 rewrites the tests/css-url-assets.test.ts header block to describe the dedicated fixtures. I confirmed the new paths now exist and the dead ones are gone:
tests/fixtures/css-url-assets-pages/styles/{global,split-a,split-b}.module.css,dark.svg,dark2.svg✓tests/fixtures/css-url-assets-app/app/page.module.css,dark.svg,dark2.svg✓- The old
pages-basic/styles/css-url-global.module.cssandapp-basic/app/nextjs-compat/css-test/url-assetsreferences are gone, and those paths no longer exist on disk.
The header, the PAGES_CSS_FIXTURE/APP_CSS_FIXTURE constants, and the in-body comment (lines 33-35) now agree. The commit touches only the comment block (8 insertions / 7 deletions, no logic change).
Full status of prior findings — all addressed
- Shared content hash on sibling files — confirmed intended Next-parity. No change needed.
assetsInlineLimit: 0blast radius — confirmed intended client-scoped parity; user config still wins (thebuild-optimization.test.tscase drivingcreateBuilderasserts this across all three environments).map: nullcomment — reworded to "the marker is transient — it's stripped before final output", no longer claiming no column shift. (index.ts:2548)assetsByBasebasename-collision — one-line comment added noting output basenames embed an 8-char content hash so last-write-wins is safe. (css-url-assets.ts:226-229)- CodeQL ReDoS —
/\/+$/replaced with a linear trailing-slash loop inclient-build-config.ts:19-21. The remaining inline annotation is on the pre-fix commitd43c21b, not the current head. configEnvironmentredundancy — confirmed as intentional belt-and-suspenders carrier for the single-build client env path; harmless.
Independently re-verified on this head
- All callers of
getClientOutputConfigForVite/createClientOutputConfig(now takingassetsDir) are updated consistently acrossindex.ts(4 sites) andclient-build-config.ts. - Build-only + client-scoped boundary holds: all three plugins are
apply: "build"and gated onthis.environment?.name === "client"— dev CSS and RSC/SSR asset handling stay untouched. - The split-chunk provenance test (
/split-a→dark,/split-b→dark2) still asserts distinct, served svgs from separate page chunks — the case the per-marker design exists for. - Nested
media/subtrees serve correctly: the Pages integration test fetches the emitted svg URLs and asserts200+image/svg+xml; the App Router test stats the files on disk.
Nothing new found. Good to merge once CI is green.
|
Review posted to PR #1725. SummaryRe-reviewed the CSS
Verdict: LGTM — no remaining nits. The only outstanding item from the last review (the stale test header comment) is fixed in All six prior findings remain addressed (map comment reworded, basename-collision comment present, ReDoS regex replaced with a linear loop, blast-radius/shared-hash confirmed intended). I independently re-confirmed: changed config signatures updated at all call sites, the build-only/client-scoped boundary holds across all three plugins, and the split-chunk provenance test still exercises distinct served svgs. Nothing new found — good to merge once CI is green. |
2 participants
Closes #1690
Closes #1703
Fixes #1450
Overview
Matches Next.js
asset/resourcebehaviour for CSSurl()dependencies: emit them as files under/_next/static/media/(instead of inlining small ones asdata:URLs), and keep byte-identical source files (e.g.dark.svg/dark2.svg) as distinct output files instead of letting Vite/Rolldown's content-based asset dedupe collapse them onto one filename.This is a slimmer reimplementation of the approach explored in #1703 — same core mechanism and public surface, roughly half the restore-side code. Co-authored with @NathanDrake2406, whose #1703 established the approach.
Why
Next.js (webpack
asset/resource) emits one output file per source file a stylesheet references, keyed by module identity. Vite/Rolldown instead dedupe emitted assets by content, so two byte-identical files collapse to a single output and everyurl()that referenced either source is rewritten to that one filename — the second filename disappears. There's no config switch to disable that dedupe (vitejs/vite#8632); the one native escape hatch isthis.emitFile({ fileName }), whose explicit-fileNameassets are never deduped.Approach
Build-only, client-scoped (dev CSS and RSC/SSR asset handling are untouched):
url()with a private?vinext_css_url_asset=<source-basename>query. This is the only durable carrier of per-reference provenance: once Rolldown dedupes, the bundle can't tell whichurl()came from which source, andoriginalFileNamesrecords the set of sources but not the per-reference mapping.assetsInlineLimit: 0(user config still wins) and route assets tocss/[name].[hash:8][extname]/media/[name].[hash:8][extname].generateBundle, read each marked reference's source basename back out; when it differs from the deduped output, emit a sibling file under that name via thefileNameescape hatch and rewrite the reference. The marker is then stripped (also from CSS that Vite inlined into JS chunks).Each reference resolves from its own marker, so split CSS chunks stay correct with no shared cursor or bundle-iteration-order dependence.
Relationship to #1703
Same mark/restore mechanism, same plugin names, same marker string, same
assetsInlineLimit/assetFileNamesconfig. The difference is the restore implementation: this drops#1703'sAssetIndexes(4 collections),reserveBundleFileNamecollision loop,restoredFileNamescache, and multi-fallbackfindAssetForUrlPath/replaceUrlPathAssetFileNamein favour of a single basename map + an emitted-set + onederiveSiblingFileName. A review concluded those guarded scenarios are unreachable given the sibling-filename construction (source-stem+ the deduped asset's own content-hash + ext), so the result is functionally equivalent with ~245 fewer lines. The one cheap robustness item the review flagged (defensivedecodeURIComponent) is included.Tests
Ports the Next.js
scss/url-globale2e into the committedpages-basicandapp-basicfixtures (built in isolation, served via the prod server):dark.*.svganddark2.*.svg; plus a cross-chunk provenance case (two routes each referencing one of the identical svgs).Validation
tests/css-url-assets.test.ts— 8/8tests/build-optimization.test.ts— 80/80prerender(79),pages-router(268; one pre-existing dev-server flake unrelated to this change, reproduces on cleanmain),routing+route-sorting(151),check(105).tscclean for the changed files; formatter applied.Note
Draft — opened for comparison/review against #1703.