Repository navigation
perf(css): write selectors, An+B and more values in their shortest spelling - #21664
Conversation
🦋 Changeset detectedLatest commit: bb98b3b The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Warning Review limit reached
Next review available in: 6 minutes 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: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Note
|
| Layer / File(s) | Summary |
|---|---|
Generated CSS metadata tooling/generate-css-data.js, test/CssValueSyntax.unittest.js, cspell.json |
The generator derives and exports nth-selector equivalent and omittable-initial-keyword tables. Tests cover the generated mappings. |
Selector and An+B normalization lib/css/syntax.js, test/CssSyntax.unittest.js, test/configCases/css/minimize-selectors/style.css |
Selector processing normalizes An+B expressions, canonicalizes selector lists, removes implied universal selectors, preserves sibling combinators, and converts equivalent keyframe positions. |
Value and declaration minification lib/css/syntax.js, test/CssSyntax.unittest.js, test/configCases/css/minimize-values/style.css |
Value minification handles zero-time conversion and additional transform reductions. Declaration minification omits eligible initial shorthand keywords and validates longhand merging. |
CSS equivalence comparison test/SyntaxBrowserEquivalence.unittest.js, .changeset/031-css-selector-sets-and-transforms.md |
Equivalence comparison canonicalizes selector formulas and selector sets, groups selectors by declaration blocks, and preserves rule ordering semantics. The changeset records a patch release. |
Possibly related PRs
- webpack/webpack#21648: Both changes modify CSS minification logic in
lib/css/syntax.js, including shorthand and transform reduction behavior.
🚥 Pre-merge checks | ✅ 3 | ❌ 1
❌ Failed checks (1 inconclusive)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Title check | ❓ Inconclusive | The title uses valid Conventional Commit syntax and accurately describes the CSS minification changes, but the branch prefix is not provided for verification. | Provide the branch name or prefix to verify that the perf type matches the required branch prefix. |
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
Comment @coderabbitai help to get the list of available commands.
|
This PR is packaged and the instant preview is available (4015bbd). Install it locally:
npm i -D webpack@https://pkg.pr.new/webpack@4015bbd
yarn add -D webpack@https://pkg.pr.new/webpack@4015bbd
pnpm add -D webpack@https://pkg.pr.new/webpack@4015bbd |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #21664 +/- ##
==========================================
- Coverage 94.52% 94.52% -0.01%
==========================================
Files 630 630
Lines 82008 82131 +123
Branches 24272 24328 +56
==========================================
+ Hits 77520 77634 +114
- Misses 4488 4497 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Generated code sizeComparing
7 asset(s) changed size
No runtime gained or lost a runtime module. Built |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
test/SyntaxBrowserEquivalence.unittest.js (1)
384-388: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReduce the new prose comments to two short lines or fewer.
test/SyntaxBrowserEquivalence.unittest.js#L384-L388: shorten theAn+Bexplanation.test/SyntaxBrowserEquivalence.unittest.js#L441-L445: shorten the selector-set explanation.test/SyntaxBrowserEquivalence.unittest.js#L1162-L1165: shorten the selector-order explanation.test/SyntaxBrowserEquivalence.unittest.js#L1211-L1213: shorten the rule-grouping explanation.As per coding guidelines, comments must be “generally one line and no more than two short lines.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/SyntaxBrowserEquivalence.unittest.js` around lines 384 - 388, Shorten the four explanatory comments in test/SyntaxBrowserEquivalence.unittest.js at lines 384-388, 441-445, 1162-1165, and 1211-1213 to two short lines or fewer, preserving only the essential explanation of the An+B, selector-set, selector-order, and rule-grouping behavior.Source: Coding guidelines
lib/css/syntax.js (1)
7901-7929: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModule-level
_mergeAtand_mergeRepeatedcouple_indexDeclarationsto its caller by ordering alone.The pair is safe today: line 9020 sets
mergeableonly when the rule has no child rules, so no nested rule is printed between_indexDeclarationsand the_mergeBoxLonghandscalls that read the state. The invariant is invisible at the definition site.Consider returning the index from
_indexDeclarationsand passing it into_mergeBoxLonghands, or record the node the index belongs to and assert it on read. The allocation the comment avoids is one small object per rule, not one per table entry.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/css/syntax.js` around lines 7901 - 7929, Decouple _indexDeclarations from its caller’s ordering by returning the per-rule merge state and passing it explicitly into _mergeBoxLonghands, or associate the module-level state with its indexed rule and validate that association when reading it. Update the related merge call sites while preserving the existing candidate and repeated-property behavior.tooling/generate-css-data.js (1)
2382-2406: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert every
omittableInitialKeywordsentry is collectedIf a future
mdn-dataupdate changesgrid-auto-flow.initialor its syntax shape,collectOmittableInitialKeywordssilently omits it. Assert that every supplement name yields an output entry, similar toassertPrimitivesExist.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tooling/generate-css-data.js` around lines 2382 - 2406, Update collectOmittableInitialKeywords to assert that every name in SUPPLEMENT.omittableInitialKeywords produces an entry in the collected output, rather than silently continuing when property metadata, initial value, or syntax shape is incompatible. Follow the assertion pattern used by assertPrimitivesExist and retain the existing collection and sorting behavior for valid entries.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/css/syntax.js`:
- Around line 6600-6611: The _minifyNth function must avoid hanging or losing
precision for unsafe B values: return null when a or b is not a safe integer,
and replace the positive-step normalization loop with the specified modulo-based
normalization. In test/CssSyntax.unittest.js lines 1409-1416, add coverage
asserting min("a:nth-child(2n-99999999999999999999){b:c}") remains unchanged.
In `@test/CssSyntax.unittest.js`:
- Around line 1409-1416: Extend the existing min() test in “keeps an An+B
selector argument no shorter spelling reaches” with an :nth-child() case whose B
magnitude exceeds Number.MAX_SAFE_INTEGER, asserting the input remains
unchanged. Use a valid large An+B expression so the regression test exercises
_minifyNth’s previously unbounded normalization loop.
In `@test/SyntaxBrowserEquivalence.unittest.js`:
- Around line 1107-1120: The nested style condition handling in conditionOf must
apply the same oneSpelling selector canonicalization as top-level labels before
sorting and joining the selector list. Update sortedSelectorList to canonicalize
each selector with oneSpelling, preserving deduplication and sorting so
equivalent An+B and pseudo-class forms produce identical chain keys.
- Around line 403-439: Update oneSpelling so NTH_CALL normalization only
processes unescaped :nth-* functional pseudo-classes in selector syntax, not
text inside quoted strings or attribute selectors. Parse or scan selector tokens
while tracking strings and attribute-value context, and apply the existing An+B
normalization only to eligible pseudo-class tokens. Preserve all literal
attribute and string contents unchanged.
In `@tooling/generate-css-data.js`:
- Around line 3619-3628: Move the emitted MERGE_LONGHANDS and
OMITTABLE_INITIAL_KEYWORDS constants to after the FAMILY_LONGHANDS block so the
PAIR_LONGHANDS comment remains directly attached to its subject. Update the
OMITTABLE_INITIAL_KEYWORDS comment to explicitly name BOX_LONGHANDS,
FAMILY_LONGHANDS, and PAIR_LONGHANDS instead of referring to tables “above.”
---
Nitpick comments:
In `@lib/css/syntax.js`:
- Around line 7901-7929: Decouple _indexDeclarations from its caller’s ordering
by returning the per-rule merge state and passing it explicitly into
_mergeBoxLonghands, or associate the module-level state with its indexed rule
and validate that association when reading it. Update the related merge call
sites while preserving the existing candidate and repeated-property behavior.
In `@test/SyntaxBrowserEquivalence.unittest.js`:
- Around line 384-388: Shorten the four explanatory comments in
test/SyntaxBrowserEquivalence.unittest.js at lines 384-388, 441-445, 1162-1165,
and 1211-1213 to two short lines or fewer, preserving only the essential
explanation of the An+B, selector-set, selector-order, and rule-grouping
behavior.
In `@tooling/generate-css-data.js`:
- Around line 2382-2406: Update collectOmittableInitialKeywords to assert that
every name in SUPPLEMENT.omittableInitialKeywords produces an entry in the
collected output, rather than silently continuing when property metadata,
initial value, or syntax shape is incompatible. Follow the assertion pattern
used by assertPrimitivesExist and retain the existing collection and sorting
behavior for valid entries.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 04d53980-f0f5-4191-b075-24ef86d83f99
⛔ Files ignored due to path filters (7)
lib/css/data.jsis excluded by!lib/css/data.jstest/configCases/css/minimize-lightningcss-selectors/__snapshots__/ConfigCacheTest.snapis excluded by!**/*.snap,!test/**/__snapshots__/**test/configCases/css/minimize-lightningcss-selectors/__snapshots__/ConfigTest.snapis excluded by!**/*.snap,!test/**/__snapshots__/**test/configCases/css/minimize-selectors/__snapshots__/ConfigCacheTest.snapis excluded by!**/*.snap,!test/**/__snapshots__/**test/configCases/css/minimize-selectors/__snapshots__/ConfigTest.snapis excluded by!**/*.snap,!test/**/__snapshots__/**test/configCases/css/minimize-values/__snapshots__/ConfigCacheTest.snapis excluded by!**/*.snap,!test/**/__snapshots__/**test/configCases/css/minimize-values/__snapshots__/ConfigTest.snapis excluded by!**/*.snap,!test/**/__snapshots__/**
📒 Files selected for processing (8)
.changeset/031-css-selector-sets-and-transforms.mdlib/css/syntax.jstest/CssSyntax.unittest.jstest/CssValueSyntax.unittest.jstest/SyntaxBrowserEquivalence.unittest.jstest/configCases/css/minimize-selectors/style.csstest/configCases/css/minimize-values/style.csstooling/generate-css-data.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
webpack/schema-utils(auto-detected)webpack/tapable(auto-detected)
Merging this PR will regress 3 benchmarks
|
`scale()` with a factor of 1 on one axis is that axis's own function, and a translation's zero component is zero in either length or percentage. A keyframe selector of 100% is the `to` CSS Animations 1 §4 names it.
A selector list is a set: the same selectors in any order match the same elements at the same specificity. Writing one canonical order lets equal lists compress alike and lets two rules naming one set join, and a repeat in it matches nothing the first already did. The equivalence harness compares a run of selectors reaching the same block under the same conditions as the set it is, which is the invariant joining them into one list already rests on.
CSS Syntax 3 §4.3.10 starts a number at `+` only before a digit, or before a `.` that itself has one. Treating every `+.` as a number made `.a+.m` print as `.a+/**/.m`, longer than the input it came from and different again on a second pass. A join also drops a selector the other side already names.
…spelling `An+B` carries its own whitespace, signs and case, an index under 1 matches nothing so a forward step starts where it does, and a step selecting exactly one child is the child Selectors 4 §14 names. A 3D matrix whose third row and column are the identity's is the 2D matrix of the six values it leaves, and a 2D pair of 1 leaves the z scale. A zero time keeps a unit where a zero length drops one, so it takes the shorter of the two it can carry. The equivalence harness reads each An+B as the sequence it selects: the engine folds `even` / `odd` itself but hands `0n+3` back as written.
A selector function's argument is a selector like the prelude's, so the universal a compound implies says nothing there either — `:not(*.g)` is `:not(.g)`. A lone universal is the selector, a parted one is a combinator away, and a namespaced one is not implied.
…ly when it is one The first/last counterpart of each An+B pseudo-class was a table typed into the printer; it is now read out of the selector table in the generator — the An+B half by its grammar, the counterpart by the name the pair builds, kept only where the table has it as a selector too. The two selector-list helpers were one function apart, and both split every prelude to find out there was nothing to do: 50,434 of the corpus's 55,055 hold no comma at all, so a list is only built where one can exist. `scale` and `rotate3d` asked whether a component was one in two different spellings; both now use the same test, which also reads `1.0`.
Every rule with two declarations was offered to three shorthand tables, and each built the same property index again: 48,204 Map and Set allocations across the corpus where one reused pair does. The table scan asked for its longhands through a closure per entry — 401,700 of them — which is now a loop. And `_isJoinablePrelude` split every prelude into a list to walk it, where 50,434 of the corpus's 55,055 hold one selector and need no list at all. Counted, not timed: the output is byte-identical on all 18 fixtures, and the wall-clock difference on this machine is inside its own run-to-run spread.
`grid-auto-flow:row dense` is the `dense` omitting its group already means. The keyword is read out of the property table and kept only where it really is the initial and really is one alternative of a top-level `||` group; which of those groups falls back to it is the judgement stated in the generator, since `aspect-ratio:auto 3/2` is a ratio with a fallback rather than the ratio alone. A rule is also offered to the three merge tables only when two of its properties are longhands one of them names — 6,238 of the corpus's 8,034 candidate rules are not, which is 311,900 table-entry checks that no longer run. The union is built in the generator out of the very arrays it emits, so it cannot drift.
`_minifyNth` read an unbounded digit run, so a `B` above `Number.MAX_SAFE_INTEGER` made `b += a` a fixed point and the normalization loop never exited. Guard both terms and normalize by modulo instead of by stepping. Also spell the two new words for cspell, keep the `PAIR_LONGHANDS` comment with its table, assert the omittable initial keyword is still one mdn-data states, and canonicalize a nested rule's selector list in the equivalence harness without rewriting text inside an attribute.
Deduplicating across the seam re-read the whole accumulated selector list once per rule joined, so a run of adjacent rules printing the same block was quadratic: 6000 of them took 2.8s where the base took 0.10s. It bought nothing measurable — no configCases stylesheet loses a byte without it — so the seam concatenates and the canonical form stays where the list is the author's own.
bad274d to
78b8574
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tooling/generate-css-data.js`:
- Around line 2360-2366: Shorten the newly added comments at the documented
locations to no more than two short lines, retaining only non-obvious safety or
derivation constraints. Preserve an explicit brief reason why the grid-auto-flow
SUPPLEMENT entry cannot be derived, and apply the same concise style to the
comments near the An+B pseudo-class and other referenced symbols.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6d94861f-3f60-42e0-8443-e9cf5504f9af
⛔ Files ignored due to path filters (1)
lib/css/data.jsis excluded by!lib/css/data.js
📒 Files selected for processing (5)
cspell.jsonlib/css/syntax.jstest/CssSyntax.unittest.jstest/SyntaxBrowserEquivalence.unittest.jstooling/generate-css-data.js
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
webpack/schema-utils(auto-detected)webpack/tapable(auto-detected)
🚧 Files skipped from review as they are similar to previous changes (4)
- cspell.json
- test/SyntaxBrowserEquivalence.unittest.js
- test/CssSyntax.unittest.js
- lib/css/syntax.js
… them A declaration's value is one component 92 times in 100 (38151 of 41387 on the Tailwind fixture), and the printer materialized the whole child list to read its length and first entry. `_valueComponents` and the four shorthand collapses now walk the child span off the node, so the list is built only where one is really needed — a custom property's text, a `unicode-range`, an attribute selector. Young-generation collections for one Tailwind minify drop from 173 to 165 under a fixed semi-space, and the emitted bytes are unchanged.
…d guard A stale Prettier reformatted eighteen template interpolations the branch never touched; Prettier 3 leaves interpolations alone, so `fmt:check` passed either way and the churn stayed in the diff. The guard that makes a spec change fail generation now takes its tables the way the other collectors do, so a test can drive it.
Summary
Runs cssnano's (1,711) and lightningcss's (1,519) own minification assertions against webpack's printer and closes every gap that is both shorter and provably equivalent, plus one defect the comparison exposed:
_wouldFuseTokenstreated any+before.as starting a number, so.a+.mprinted as.a+/**/.m— longer than its input, and different again on a second pass.A selector list is now written as the set it is (one order, no repeats),
An+Bin its shortest spelling with:nth-child(1)naming:first-child,matrix3dreduced tomatrixwhere the 3D part is the identity, the implied universal dropped inside:not()/:is(), a zero time carrying the shorter of its two units, and a component spelling its property's own initial dropped. Every rewrite was adjudicated in headless Chromium before it shipped; several that cssnano and lightningcss perform were rejected there and are recorded as declined instead.yarn test:sizemoves 15 of theconfigCasesassets, all downward: −1,453 raw / −514 gzip across an 18-stylesheet framework corpus (Bootstrap, Tailwind, Bulma, Foundation, Material, Semantic UI, Pico, Tachyons, UIkit, Font Awesome, …), with no class selector lost by any fixture.Two commits are pure refactor and change no output: the
An+Bnamed equivalents and the merge-table longhand union are now derived intooling/generate-css-data.jsrather than typed into the printer, and the hot paths allocate less — a rule's declarations are indexed once for all three merge tables instead of three times (48,204 Map/Set allocations and ~401,700 closures removed per corpus pass), the tables are skipped entirely for the 78% of candidate rules that cannot match one (311,900 fewer entry checks), and a selector list is only built for the 8.4% of preludes that hold a comma.What kind of change does this PR introduce?
perf — with one fix (the empty comment after a sibling combinator) and two output-neutral refactors.
Did you add tests for your changes?
Yes —
test/CssSyntax.unittest.jsandtest/CssValueSyntax.unittest.js(unit, including the new generator collectors), andtest/configCases/css/minimize-selectors/andtest/configCases/css/minimize-values/(integration, each new case paired with the declined one next to it).test/SyntaxBrowserEquivalence.unittest.jsreads a selector list and anAn+Bas what they select rather than how they are written, which is the invariant the rewrites rest on; I mutation-tested that change — dropping a selector, flippingoddtoeven, and shifting theAn+Blift each make it fail.Does this PR introduce a breaking change?
No. Every rewrite is equivalent to the engine;
ConfigTestCasesandConfigCacheTestCasespass unchanged apart from the snapshots of the cases above.If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a — no option or public API changes. The transforms webpack declines, and why, are documented next to them in
lib/css/cssMinify.jsandtooling/generate-css-data.js.Use of AI
Yes. Claude Code was used throughout: to extract and replay the upstream cssnano and lightningcss assertions against webpack's printer, to write the implementation and tests, and to run the measurements quoted above. Every claimed equivalence was checked in headless Chromium rather than assumed, and the ones that failed there were dropped —
transitionfamily merging (it clobbers an earliertransition-behavior),translate(var(--x),0)reduction (it revives a declaration the browser drops),background:none→0 0, and0%→0onbackground-position. All output and measurements were reviewed by me before pushing.Generated by Claude Code
Summary by CodeRabbit
Bug Fixes
An+Bselectors, selector lists, and selector functions.Performance & Output Improvements