fix(canonicalize): collapse arbitrary values into shorthand utilities#19837
Conversation
36e6216 to
ef598fd
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
WalkthroughThe PR relaxes a filter in 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tailwindcss/src/canonicalize-candidates.test.ts (1)
1314-1328: Add a stream-state regression assertion for this arbitrary-value case.This test validates collapse, but it doesn’t yet verify the reported stream-order behavior (same input before/after prior calls). Adding that check here would lock in the
#19835fix more directly.Suggested test strengthening
test('collapse canonicalization works for arbitrary values', { timeout }, async () => { let designSystem = await designSystems.get(__dirname).get(css` `@import` 'tailwindcss'; `) let options: CanonicalizeOptions = { collapse: true, logicalToPhysical: true, rem: 16, } - expect( - designSystem.canonicalizeCandidates(['px-[1.2rem]', 'py-[1.2rem]', 'text-left'], options), - ).toEqual(['text-left', 'p-[1.2rem]']) + let target = ['px-[1.2rem]', 'py-[1.2rem]', 'text-left'] + + // First encounter should already collapse + expect(designSystem.canonicalizeCandidates(target, options)).toEqual(['text-left', 'p-[1.2rem]']) + + // Prior calls in the same process should not affect the result + designSystem.canonicalizeCandidates(['p-[1.2rem]'], options) + designSystem.canonicalizeCandidates(['mb-4', 'text-sm'], options) + + expect(designSystem.canonicalizeCandidates(target, options)).toEqual(['text-left', 'p-[1.2rem]']) })🤖 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 1314 - 1328, Add a regression assertion that the stream/state is stable by calling designSystem.canonicalizeCandidates a second time with the same inputs and options and asserting the result is identical; specifically, after the existing expect(...) for designSystem.canonicalizeCandidates(['px-[1.2rem]', 'py-[1.2rem]', 'text-left'], options), call designSystem.canonicalizeCandidates(...) again and assert it returns the same array (['text-left','p-[1.2rem]']), using the same designSystem (from designSystems.get) and options to lock in the stream-order fix for canonicalizeCandidates.
🤖 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 1314-1328: Add a regression assertion that the stream/state is
stable by calling designSystem.canonicalizeCandidates a second time with the
same inputs and options and asserting the result is identical; specifically,
after the existing expect(...) for
designSystem.canonicalizeCandidates(['px-[1.2rem]', 'py-[1.2rem]', 'text-left'],
options), call designSystem.canonicalizeCandidates(...) again and assert it
returns the same array (['text-left','p-[1.2rem]']), using the same designSystem
(from designSystems.get) and options to lock in the stream-order fix for
canonicalizeCandidates.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ccb1b508-393e-4c79-86d1-294cb62daffb
📒 Files selected for processing (2)
packages/tailwindcss/src/canonicalize-candidates.test.tspackages/tailwindcss/src/canonicalize-candidates.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/tailwindcss/src/canonicalize-candidates.ts
`dynamicUtilities` discovers collapse targets by swapping utility roots (e.g., `px` → `p`), but the guard at line 326 restricted this to named values. This was likely intentional for the initial implementation, but `cloneCandidate` and `printCandidate` both handle arbitrary values correctly, so the restriction can be safely relaxed. Without this, arbitrary values like `px-[1.2rem] py-[1.2rem]` were never collapsed into `p-[1.2rem]` — which is what caused the non-deterministic `--stream` output reported in tailwindlabs#19835. Fixes tailwindlabs#19835
ef598fd to
e3bba73
Compare
|
Thanks for improving and merging! 🙏 |
This PR adds more declaration expansions such that we can collapse more utilities. While testing #19837 I noticed that in my tests some utilities weren't canonicalized correctly. As part of that PR, we check for `parsedCandidate.value === null`, which means that a functional utility without a value is skipped. We do have utilities like that such as `border` (which is equivalent to `border-1`). But while testing, I noticed that `border-x border-y` should collapse to `border` but they didn't. This PR fixes that. By expanding these properties to their long-form physical properties (instead of the shorter logical properties) we make the signatures of utilities a bit bigger, but also more correct such that we can collapse the physical form into logical utilities. To make this more concrete, this PR allows for the following canonicalizations now: | Input | Output | | --- | --- | | `border-t-123 border-r-123 border-b-123 border-l-123` | `border-123` | | `border-t-1 border-r-1 border-b-1 border-l-1` | `border` | | `border-t-123 border-b-123` | `border-y-123` | | `border-l-123 border-r-123` | `border-x-123` | | `border-t-red-500 border-r-red-500 border-b-red-500 border-l-red-500` | `border-red-500` | | `border-t-red-500 border-b-red-500` | `border-y-red-500` | | `border-l-red-500 border-r-red-500` | `border-x-red-500` | | `scroll-mt-123 scroll-mr-123 scroll-mb-123 scroll-ml-123` | `scroll-m-123` | | `scroll-mt-123 scroll-mb-123` | `scroll-my-123` | | `scroll-ml-123 scroll-mr-123` | `scroll-mx-123` | | `scroll-pt-123 scroll-pr-123 scroll-pb-123 scroll-pl-123` | `scroll-p-123` | | `scroll-pt-123 scroll-pb-123` | `scroll-py-123` | | `scroll-pl-123 scroll-pr-123` | `scroll-px-123` | | `overflow-x-hidden overflow-y-hidden` | `overflow-hidden` | | `overscroll-x-contain overscroll-y-contain` | `overscroll-contain` | ## Test plan 1. Existing tests pass 2. Added a few more tests to verify that these canonicalizations work
The guard on
dynamicUtilitiesrestricted root-swapping to named values, so arbitrary values likepx-[1.2rem] py-[1.2rem]were never collapsed intop-[1.2rem].This is what caused #19835 — in
--streammode, the collapse only happened if an earlier line caused the shorthand to be registered inSTATIC_UTILITIES_KEYas a side effect, making the output non-deterministic. The underlying issue is that arbitrary value collapse wasn't supported at all.The fix relaxes the guard from
parsedCandidate.value?.kind !== 'named'toparsedCandidate.value === null.cloneCandidateandprintCandidatealready handle arbitrary values, so the root-swapping machinery works without other changes.The iteration in
dynamicUtilitiesis overdesignSystem.utilities.keys('functional')— a fixed set of roots, not input-proportional — so the performance cost of including arbitrary values should be negligible.Fixes #19835.