Canonicalization: combine text-* and leading-* classes
#19396
Conversation
Later declarations will win, e.g.:
```
.foo {
color: red;
color: blue;
}
```
↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓↓
```
.x {
color: blue;
}
```
It was already an array and we're not mutating it during each for-loop either so there is no need to create an unnecessary copy.
We were combining utilities that were setting the same property _and_ the same value before testing. This is correct for most of the utilities, but it will break down the moment you want to combine values like this: ``` text-sm/6 leading-7 ``` The `text-sm/6` would set a `line-height: 24px` but the `leading-7` would set a `line-height: 28px`. Yet, if we combine both utilities the `leading-7` will win, and the winning combination will become `text-sm/7`. While this will lead to more combinations to try, the list will still be limited to a handful.
This will allow us to re-use the cached logic and allows different spacing values like `--spacing: 0.25rem` or `--spacing: 4px`. This will also canonicalize the incoming value such that an incoming `px` value and a `rem` based spacing value can be compared. This conversion only works if the `options.rem` is provided. Canonicalization of expressions still happen regardless.
In the unlikely event that the `--spacing` value ends up to be `0`, then we would get a divide by 0 error. Or in JavaScript... Infinity. The bigger advantage is that we don't even have to parse the incoming input when the spacing value is `0` because it will be invalid anyway.
WalkthroughAdds a changelog entry and a new cartesian product utility. Tests for canonicalization were reorganized and expanded with data-driven cases. API updates: Pre-merge checks✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/tailwindcss/src/canonicalize-candidates.ts (3)
110-123: ThreadCanonicalizeOptionsconsistently into storage prep and tighten SPACING cache behaviorYou’re now letting
prepareDesignSystemStorage/createSpacingCacheclose overoptions?.remand using that for both the root--spacingmultiplier and individual lookups, which is exactly what’s needed for rem-aware canonicalization. A couple of details are worth tightening:
SPACING_KEY lifetime vs options.rem
SPACING_KEYis cached once perDesignSysteminstance and created via??=:designSystem.storage[SPACING_KEY] ??= createSpacingCache(designSystem, options)createSpacingCachebakesoptions?.reminto the returnedDefaultMap(both for the root multiplier and for eachinputlookup).- However,
createCanonicalizeOptionsandcanonicalizeCandidatesstill call:without passinglet designSystem = prepareDesignSystemStorage(baseDesignSystem)options. If any code path callsprepareDesignSystemStoragefirst without options (or with a differentrem) beforecreateSignatureOptionsruns,SPACING_KEYwill be initialised with that “first”remand never rebuilt, even if later canonicalization requests pass a differentrem.This is probably fine if a
DesignSystemis only ever used with a singleCanonicalizeOptionsinstance, but it’s an implicit invariant. To make this more robust and self-documenting, I’d recommend:function createCanonicalizeOptions( baseDesignSystem: BaseDesignSystem, signatureOptions: SignatureOptions, options?: CanonicalizeOptions, ) { let features = Features.None if (options?.collapse) features |= Features.CollapseUtilities
- let designSystem = prepareDesignSystemStorage(baseDesignSystem)
- let designSystem = prepareDesignSystemStorage(baseDesignSystem, options)
}export function canonicalizeCandidates(
baseDesignSystem: BaseDesignSystem,
candidates: string[],
options?: CanonicalizeOptions,
): string[] {
let signatureOptions = createSignatureOptions(baseDesignSystem, options)
let canonicalizeOptions = createCanonicalizeOptions(baseDesignSystem, signatureOptions, options)
- let designSystem = prepareDesignSystemStorage(baseDesignSystem)
- let designSystem = prepareDesignSystemStorage(baseDesignSystem, options)
That guarantees the first `prepareDesignSystemStorage` call for a given `DesignSystem` instance sees the same `CanonicalizeOptions` that drove `signatureOptions`, and prevents subtle mismatches if someone reuses a `DesignSystem` with different `rem` values later. If you foresee truly different `rem` values against the same `DesignSystem`, an alternative would be to key the spacing cache by `rem` (e.g. `DefaultMap<number | null, DefaultMap<string, number | null>>`) instead of closing over it, but the above change is a minimal fix. 2. **Spacing cache behavior when `--spacing` is 0** The short-circuit in `createSpacingCache`: ```ts if (value === 0) return nullis good to avoid division by zero, but it also means a theme with
--spacing: 0effectively disables spacing-based canonicalization. That’s probably acceptable, just calling it out as an intentional tradeoff.
Type / sign of
bareValuewhen turning arbitrary functional values into namedIn
arbitraryUtilities’stryReplacementsyou now do:let bareValue = designSystem.storage[SPACING_KEY]?.get(value) ?? null if (bareValue !== null) { if (isValidSpacingMultiplier(bareValue)) { yield Object.assign({}, candidate, { value: { kind: 'named', value: bareValue, fraction: null }, }) } }
SPACING_KEYreturns a numeric ratio (number | null), andNamedUtilityValue['value']is typically treated as a string elsewhere (e.g.tryValueReplacementsalways setsvalue: string). To keep the representation consistent (and avoid any TS friction), it’d be safer to normalise this:
- if (bareValue !== null) {
if (isValidSpacingMultiplier(bareValue)) {yield Object.assign({}, candidate, {value: { kind: 'named', value: bareValue, fraction: null },})}- }
- if (bareValue !== null && isValidSpacingMultiplier(bareValue)) {
let normalized = String(bareValue)yield Object.assign({}, candidate, {value: { kind: 'named', value: normalized, fraction: null },})- }
- If
valuecould ever be negative,SPACING_KEYwill return a negative ratio;isValidSpacingMultipliercurrently accepts that, and you’d end up with a named value like-4. That likely isn’t what you want (you already handle signed multipliers viarootPrefixfurther down when usingspacingMultiplier). If negative inputs are possible here, consider guarding onbareValue >= 0and letting the existingspacingMultiplier/rootPrefixlogic own the signed case.Also applies to: 150-150, 166-177, 179-188, 936-957, 1064-1070
261-309: Text/leading precompute optimization looks sound; consider aligning comments and documenting the property-only intersectionThe new hard-coded optimization around
line-height/font-sizeandtext-*precomputation incollapseGroupis a nice way to surfacetext-size/line-heightcombos forcollapseCandidateswithout bloating the generic path. A few minor, mostly documentation-level nits:
Property-only intersection vs outdated comment
The comment above still says:
// For each property, lookup other utilities that also set this property and // this exact value. If multiple properties are used, use the intersection of // each property.But the implementation now intentionally unions over all values per property:
for (let property of propertyValues.keys()) { let otherUtilities = new Set<string>() for (let group of staticUtilities.get(property).values()) { for (let candidate of group) { otherUtilities.add(candidate) } } ... result = intersection(result, otherUtilities) }That’s the right move for cases like
text-sm/6 leading-7, where values differ but the final cascaded CSS is representable as a singletext-*utility. It’d be good to update the comment to say “same property” rather than “same property and value” to avoid confusing future readers.Reliance on signatures for safety
With property-only intersections, the “is this collapse safe?” check now relies entirely on:
let collapsedSignature = signatures.get(comboKey) ... let signature = signatures.get(replacement) if (signature !== collapsedSignature) continueThat’s correct and matches the PR description (“last declaration wins” semantics), but it’s worth documenting somewhere near this block that we deliberately ignore value-level equality during candidate discovery and let signature equality gate correctness.
Micro-optimisation (optional)
fontSizeNames = designSystem.theme.keysInNamespaces(['--text'])combined withinterestingLineHeightscan lead toO(|text-sizes| * |line-heights|)precomputation for every group with at least oneline-height. That’s probably fine given typical scales, but if this ever becomes hot, you could consider restrictingfontSizeNamesto the sizes actually present incandidatePropertiesValues(the second loop already does this for arbitrary font-size values).Overall the optimization is well-contained and matches the intended
text-*/leading-*canonicalization behavior.Also applies to: 318-332, 345-347
2141-2165: Declaration dedup + sort incanonicalizeAstworks; tighten sort behavior for mixed node kindsThe new
exitlogic incanonicalizeAst:
- Walks declarations from the end and removes earlier declarations of the same
property, keeping only the last one.- Then sorts
node.nodeslexicographically bypropertyfor declarations.This is a good fit for signature computation:
- It matches the “last declaration wins” behavior needed to treat, e.g.,
leading-7 text-smandtext-sm/7as equivalent when their final CSS matches.- Removing earlier fallbacks also simplifies signatures so we can reason at the “final behaviour” level rather than caring about legacy value sequences.
Two small concerns / suggestions:
Comparator behavior for non-declaration children
node.nodes.sort((a, b) => { if (a.kind !== 'declaration') return 0 if (b.kind !== 'declaration') return 0 return a.property.localeCompare(b.property) })
If
node.nodescan ever contain a mix of declarations and other node types (e.g. nested rules/at-rules inside an at-rule), this comparator doesn’t define a strict ordering for comparisons involving non-declarations (always returns 0). That means the relative order of declarations vs non-declarations is effectively left to the JS engine’s sort implementation.Since these ASTs are only used for signatures, changing that order probably doesn’t break runtime CSS, but for determinism it’d be safer to make the intent explicit, e.g.:
node.nodes.sort((a, b) => { let aIsDecl = a.kind === 'declaration' let bIsDecl = b.kind === 'declaration' if (aIsDecl && bIsDecl) { return a.property.localeCompare(b.property) } if (aIsDecl !== bIsDecl) { // Always keep non-declarations before declarations (or vice versa) return aIsDecl ? 1 : -1 } return 0 })This preserves non-declaration relative ordering while still providing deterministic ordering for declarations.
Fallback sequences vs canonical equivalence
By dropping earlier declarations of the same property, you’re intentionally treating:
.x { color: legacy-fallback; color: final; }and
.x { color: final; }as the same signature. That matches the “canonicalization equals final cascaded result” philosophy used elsewhere in this file, but it might be worth adding a brief comment to that effect above the dedup block so it’s clear this is a deliberate tradeoff (and not an accidental loss of fallback information).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)packages/tailwindcss/src/canonicalize-candidates.test.ts(3 hunks)packages/tailwindcss/src/canonicalize-candidates.ts(8 hunks)packages/tailwindcss/src/cartesian.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/tailwindcss/src/canonicalize-candidates.test.ts (1)
packages/tailwindcss/src/cartesian.ts (1)
cartesian(10-40)
packages/tailwindcss/src/canonicalize-candidates.ts (5)
packages/tailwindcss/src/intellisense.ts (1)
CanonicalizeOptions(7-7)packages/tailwindcss/src/design-system.ts (1)
DesignSystem(33-65)packages/tailwindcss/src/utils/infer-data-type.ts (1)
isValidSpacingMultiplier(356-358)packages/tailwindcss/src/constant-fold-declaration.ts (1)
constantFoldDeclaration(8-115)packages/tailwindcss/src/utils/default-map.ts (1)
DefaultMap(5-20)
🔇 Additional comments (4)
CHANGELOG.md (1)
23-23: Changelog entry is clear and consistentThe new “Canonicalization: combine
text-*andleading-*classes” entry is accurate, matches surrounding style, and correctly references the PR number.packages/tailwindcss/src/canonicalize-candidates.test.ts (3)
5-65: cartesian import and test name formatting look goodImporting
cartesianfrom./cartesianis correct for the new helper, and the simplifiedtestName = '%s → %s (%#)'still preserves useful information in Vitest output while avoiding extra backticks. No issues here.
1029-1057: Shorthand-combination tests are well targetedThe new “combine to shorthand utilities” block exercises key canonicalization paths (margins to
m-*/my-*,w+htosize-*, arbitrary props to shorthand, and thetext-sm/relaxedcase) and correctly usesexpectCombinedCanonicalizationso multiple candidates are canonicalized together. The behavior around differing variants (e.g.hover:w-4 h-4) is also covered. Looks solid.
1059-1091: cartesian‑driven font-size/line-height coverage is robustUsing
cartesianto generate all combinations of equivalent font-size and line-height forms gives thorough coverage of the newtext-{x}/{y}canonicalization without duplicating test data. Trimming the padded candidate string before splitting ensures the extra spaces only affect test titles, not behavior. The expected outputs (text-sm/7,text-[15px]/7, etc.) align with the configured theme values earlier in the file. No issues.
This PR improves the canonicalization when using
text-*andleading-*utilities together.When using classes such as:
Then the canonical way of writing this is:
Similarly, if you already have a modifier applied, and add a new line-height utility. It will also combine them into the canonical form:
becomes:
This is because the final CSS output of
text-sm/6 leading-7is:Where the
line-heightof theleading-7class wins over theline-heightof thetext-sm/6class.Implementation
On the fly pre-computation
Right now, we are not using any AST based transformations yet and instead rely on a pre-computed list. However, with arbitrary values we don't have pre-computed values for
text-sm/123for example.What we do instead is if we see a utility that sets
line-heightand other utilities setfont-sizethen we pre-compute those computations on the fly.We will prefer named font-sizes (such as
sm,lg, etc). We will also prefer bare values for line-height (such as7) over arbitrary values (such as[123px]).Canonicalization of the CSS AST
Another thing we had to do is to make sure that when multiple declarations of the same property exist, that we only keep the last one. In the real world, multiple declarations of the same value is typically used for fallback values (e.g.:
background-color: #fff; background-color: oklab(255 255 255 / 1);).But for our use case, I believe we can safely remove the earlier declarations to make the most modern and thus the last declaration win.
Trying combinations based on
propertyonlyOne small change we had to make is that we try combinations of utilities based on property only instead of property and value. This is important for cases such as
text-sm/6 leading-7. These 2 classes will set alin-heightof24pxand28pxrespectively so they will never match.However, once combined together, there will be 2 line-height values, and the last one wins. The signature of
text-sm/6 leading-7becomes:↓↓↓↓↓↓↓↓↓
This now shows that just
text-sm/7is the canonical form. Because it produces the same final CSS output.Test plan
text-*andleading-*utilities with named, bare and arbitrary values. Even with existing modifiers on the text utilities.