Repository navigation
Conversation
In a CSS module, the argument of :global(...) was parsed with the same path as any other selector, so every class and id inside it was registered as a local symbol. The printer hid that by dropping the css_module while printing :global, but the JS exports object is built from the local scope, so those names were exported under hashed names that no rule in the output uses. SelectorParser now tracks whether it is inside :global(...) (a nested :local(...) switches back), and classes and ids parsed there stay plain identifiers: printed as written, never registered, never exported. The printer no longer special-cases :global, so :local(...) nested inside :global(...) prints the generated name it is exported under.
|
Warning Review limit reached
Next review available in: 23 seconds Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
|
Status: ready for review. Reproduced on Bun 1.4.0 and on main with |
There was a problem hiding this comment.
LGTM — moves the local/global scoping decision to parse time so the exports object and printed CSS agree, matching esbuild.
What was reviewed:
in_global_scopesave/restore inparse_css_module_scoped_selector:mem::replace+ unconditional restore before?, so nested:local()and:is()error-recovery both hand back the enclosing scope.- All three
SelectorParser { .. }construction sites initialize the new field; no others exist. - Printer's merged
Local/Globalarm:write_ident_or_ref→lookup_ident_or_refalready branches onIdentvsRef, so global names (now plainIdent) print as-written without the oldcss_module.take()workaround. - The removed exports were phantom (hashed names matching no rule in the output), so this is a bugfix, not a break.
Extended reasoning...
Overview
The PR fixes CSS-module scoping for :global(...) and :local(...). Previously, classes/ids inside :global(...) were registered as local symbols during parsing (so they appeared in the JS exports object under hashed names), while the printer worked around this by clearing dest.css_module when serializing the :global argument. That left the exports object out of sync with the printed CSS, and made :local(...) nested inside :global(...) print unhashed while still being exported hashed. The fix adds an in_global_scope flag on SelectorParser, toggled by :global(...)/:local(...) around their argument, so new_local_identifier produces a plain Ident (not a symbol Ref) for global-scoped names. The printer's Global arm is then merged with the Local arm since scoping is decided per-component at parse time. Three SelectorParser struct-literals in css_parser.rs initialize the new field to false.
Security risks
None. This is bundler output shaping for CSS modules — no untrusted-input parsing changes, no allocation/size arithmetic changes, no FFI.
Level of scrutiny
Medium. It changes user-visible bundler output (the shape of the default export from a .module.css import), but the removed keys mapped to hashed names that no rule in the output CSS used, so any consumer relying on them was already broken. The change aligns Bun with esbuild's local-css semantics, which is the reference for this feature. The Rust change is ~30 net lines and mechanical.
Other factors
Test coverage is thorough: seven new itBundled cases covering the primary export-shape fix, nested :local inside :global, error-path scope restoration under :is() recovery, same name used both locally and globally, nested rule blocks not inheriting the paren scope, an all-global module exporting nothing, and composes of a global-only name now erroring. One existing esbuild-parity test is tightened to also expect the y error (esbuild errors on it too). I confirmed via grep that only three SelectorParser { .. } literals exist and all initialize in_global_scope: false, and that lookup_ident_or_ref already handles the plain-Ident branch independent of css_module, so dropping the printer's css_module.take() is safe.
| /// CSS modules: set while parsing the argument of `:global(...)` and | ||
| /// cleared again inside a nested `:local(...)`. Classes and ids parsed in | ||
| /// this state keep their written name instead of becoming local symbols, | ||
| /// so they are neither renamed nor exported. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| /// Parses the argument of `:local(...)` / `:global(...)`. The scope covers | ||
| /// only the argument: it is restored even when the argument fails to parse, | ||
| /// because `:is()`-style error recovery keeps parsing the rest of the | ||
| /// selector list with this same parser. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
| // The parser already scoped the classes and ids inside these: a | ||
| // local one is a `Ref` (printed under its generated name), a | ||
| // global one is a plain `Ident` (printed as written). That also | ||
| // covers `:local(...)` nested inside `:global(...)`. |
There was a problem hiding this comment.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code
|
#45487 replaces this PR. It makes the same change at parse time: a name inside This branch should not merge as it is. Its printer change removes the only thing that keeps a view-transition name plain inside |
Problem
:global(...)is printed unhashed in the stylesheet (correct), but it is also exported from the JS exports object, under a hashed name that no rule in the output uses. For:global(.g) {}plus.local :global(.h) {},import styles from "./w.module.css"gives{ g: "g_V1RUew", local: "local_V1RUew", h: "h_V1RUew" }while the CSS contains.gand.local_V1RUew .h. esbuild exports{ local }only.:local(...)nested inside:global(...)had the opposite mismatch::global(.a :local(.b))printed.a .bbut exportedb: "b_V1RUew"(esbuild prints.a .b_hash).:global(...)could becomposesd, producing the same phantom hashed name in the export.:global(...)(src/css/selectors/parser.rs,parse_non_ts_functional_pseudo_class) parses its argument with the ordinary selector path, soparse_one_simple_selectorrunsnew_local_identifierfor every class and id inside it and registers them in the sheet'slocal_scope. The exports object is built fromlocal_scope(src/bundler/linker_context/generateCodeForLazyExport.rs). The printer only hid this on the CSS side: thePseudoClass::Globalarm in src/css/selectors/selector.rs cleareddest.css_modulewhile printing its argument, which made those refs print under their original names (the fallback added by fix(css)::globalin css modules #18257 for CSS Modules:global()selector causes crash #18170, which was the crash this caused before).Fix
SelectorParsergets anin_global_scopeflag.:global(...)sets it and:local(...)clears it for the duration of their argument (restored afterwards, also on a parse error, since:is()-style recovery keeps going with the same parser).new_local_identifierreturns a plain identifier instead of registering a symbol while the flag is set, so global names are never inlocal_scope: not renamed, not exported, not composable.Globalarm now prints its argument like theLocalarm does. Scoping is decided once, at parse time, per class/id (local =Ref, global =Ident), and both the stylesheet and the exports object follow that one decision, which is also what makes the nested:local(...)case print the name it is exported under.local-csssemantics for all of the above (:global()argument is global, a nested:local()is local again, a global-only name cannot be composed), and it is what the exports object is documented to be: a map from the file's local names to the generated names.css/ImportCSSFromJSComposesFromMissingImport(test/bundler/esbuild/css.test.ts) now also expects the error fory, which that test defines only as:global(.y); esbuild errors on it too.GlobalPseudoFunctionNamesAreNotExported,LocalPseudoFunctionInsideGlobal,GlobalScopeRestoredAfterInvalidArgument,GlobalAndLocalUseOfSameName,GlobalPseudoFunctionDoesNotLeakIntoNestedRules,OnlyGlobalSelectorsExportsNothing,ComposesNameOnlyDefinedAsGlobal). Without the src change 5 of them fail (plus the esbuild test); with it all pass.cargo clippy -p bun_css.Background
*.module.css, every class and id selector is parsed into aRefto a per-file symbol (Parser::add_symbol_for_name, which also records the name in the sheet'slocal_scope). The linker later picks a generated name per symbol (mangle_local_css), the CSS printer looks refs up in that map, and the JS side of the import is an object literal with one entry perlocal_scopeentry. A selector component can instead hold a plainIdent, which the printer writes as is; that is what every class in a non-module stylesheet is, and now also what a global name in a module is.:global(...)/:local(...)are the CSS modules pseudo-classes for opting a piece of a selector out of (or back into) local scoping. The wrappers themselves are not printed; only their argument is.Printer.css_moduleis the printer's CSS-modules state; it isSomefor the whole of a module stylesheet. Class/id components use it to decide between printing the generated name (lookup_symbol) and the original one, which is the switch the oldGlobalarm flipped.Repro before / after (
bun build entry.js --outdir out)w.module.css:Bun 1.4.0 exports:
and prints
.g,.local_V1RUew .h,.a .b,.c_V1RUew,#gid_V1RUew,#gid2.With this change the exports are
local,b,c,gid, and the third rule prints as.a .b_V1RUew. esbuild 0.25.1 on the same input exportslocal,b,c,gidand prints.a .w_b.