Canonicalization: do not canonicalize units in arbitrary values#19988
Canonicalization: do not canonicalize units in arbitrary values#19988RobinMalfait merged 3 commits intomainfrom
Conversation
When creating signatures, we do want to normalize units into a stable unit such that `20in` becomes `1920px`. However, when we are optimizing arbitrary values, we don't want to do that because this is used as an escape hatch, and the unit is very likely to be chosen for a particular reason. That said, arbitrary values should still get canonicalized (e.g. no need to have multiple calc expressions)
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds regression tests covering arbitrary inset spacing values with inch units. Introduces a 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tailwindcss/src/canonicalize-candidates.test.ts (1)
1214-1225: Broaden the regression a bit.This locks down the
incase, but the new behavior is generic across all convertible length units. Adding one more case for a different unit, such ascmorrem, would better protect thenormalizeUnit=falsepath from regressions.Suggested addition
['-mt-[0.04in]', 'mt-[-0.04in]'], ['mt-[-0.04in]', 'mt-[-0.04in]'], ['-mt-[-0.04in]', 'mt-[0.04in]'], + ['-mt-[0.1cm]', 'mt-[-0.1cm]'],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tailwindcss/src/canonicalize-candidates.test.ts` around lines 1214 - 1225, Add a new test case to the existing test.each block (the table driving the test with test.each and the call to expectCanonicalization) to cover a different length unit (e.g., use 'cm' or 'rem') so the normalization behavior with normalizeUnit=false is protected more generally; mirror the three-case pattern used for 'in' (e.g., ['-mt-[0.04cm]', 'mt-[-0.04cm]'], ['mt-[-0.04cm]', 'mt-[-0.04cm]'], ['-mt-[-0.04cm]', 'mt-[0.04cm]']) and place it alongside the existing 'in' cases in the same test.each array.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/tailwindcss/src/canonicalize-candidates.test.ts`:
- Around line 1214-1225: Add a new test case to the existing test.each block
(the table driving the test with test.each and the call to
expectCanonicalization) to cover a different length unit (e.g., use 'cm' or
'rem') so the normalization behavior with normalizeUnit=false is protected more
generally; mirror the three-case pattern used for 'in' (e.g., ['-mt-[0.04cm]',
'mt-[-0.04cm]'], ['mt-[-0.04cm]', 'mt-[-0.04cm]'], ['-mt-[-0.04cm]',
'mt-[0.04cm]']) and place it alongside the existing 'in' cases in the same
test.each array.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d38c7926-754f-4620-a37c-b1fe69add920
📒 Files selected for processing (3)
packages/tailwindcss/src/canonicalize-candidates.test.tspackages/tailwindcss/src/canonicalize-candidates.tspackages/tailwindcss/src/constant-fold-declaration.ts
This PR improves the canonicalization when dealing with arbitrary values.
As part of the canonicalization process we compute a signature for a given utility. This way we can ensure that when we canonicalize a candidate into a simpler candidate that it's still equivalent if the signatures match.
One thing we do during signature computation is normalizing dimensions (value + unit) into the same unit to make comparisons easier.
For example:
Will both get converted to
1920pxand thereforefooandbarwill have the same signature.Up until this part, everything is fine. However, this normalization also leaks when we try to canonicalize arbitrary values. One of the things we do is try to move the
-into the arbitrary value:This is obviously not cleaner, but we can perform some canonicalization of the arbitrary value. As part of that we do constant folding and the normalization of base units. That means that we would see this:
But this might be very confusing because it might not make sense where the
1920pxeven came from... the only thing we should have done here is constant fold that calc expression.That's what this PR does, it only does the constant folding but without the unit normalization:
Which is exactly what we want!
Fixes: tailwindlabs/tailwindcss-intellisense#1573
Test plan