Skip to content

Commit 5a4a7eb

Browse files
fix(canonicalize): prevent collapse cache pollution across calls (#19675)
## Summary fixes schoero/eslint-plugin-better-tailwindcss#321 This PR fixes an order-sensitive canonicalization bug. This bug caused issues when running eslint-plugin-better-tailwindcss as the order in which files are linted in is not consistent. This caused, in some scenarios, `canonicalizeCandidates(..., { collapse: true, logicalToPhysical: true, rem: 16 })` to stop collapsing valid combinations (for example `h-4 + w-4 -> size-4`) after unrelated prior calls. To reproduce this issue: ``` # checkout this branch $ git checkout c/fix-canonicalizeCandidates # Revert the fix to the current `main` branch $ git checkout main ./packages/tailwindcss/src/canonicalize-candidates.ts # Run the tests $ pnpm run test ``` This should produce a failure like so: ``` FAIL tailwindcss src/canonicalize-candidates.test.ts > regressions > collapse canonicalization is not affected by previous calls AssertionError: expected [ 'underline', 'h-4', 'w-4' ] to deeply equal [ 'underline', 'size-4' ] - Expected + Received [ "underline", - "size-4", + "h-4", + "w-4", ] ❯ src/canonicalize-candidates.test.ts:1167:66 1165| designSystem.canonicalizeCandidates(['underline', 'mb-4'], options) 1166| 1167| expect(designSystem.canonicalizeCandidates(target, options)).toEqual(['underline', 'size-4']) | ^ 1168| }) 1169| }) ``` ``` # reset all changes on this branch git reset --hard # run the tests again (they should now pass) pnpm run test ``` The cause of this bug is that the canonicalization caches used `DefaultMap` in places where lookups were expected to be read-only. `DefaultMap.get` inserts missing entried, which mutated shared cache state during intermediate lookups and made later canonicalization results depend on prior calls. By replacing the use of `DefaultMap` with a plain `Map`, it avoids inserting into the map on lookup paths. I've polyfilled [`Map#getOrInsert`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map/getOrInsert) as it is not widely available yet, and used that where appropriate. ## Test plan I wrote a test that fails on `main` branch, I then fixed the issue, and validated that the test now passes. --------- Co-authored-by: Robin Malfait <[email protected]>
1 parent d520e1f commit 5a4a7eb

3 files changed

Lines changed: 48 additions & 0 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
3232
- Improve performance Oxide scanner in bigger projects ([#19632](https://github.com/tailwindlabs/tailwindcss/pull/19632))
3333
- Ensure import aliases in Astro v5 work without crashing ([#19677](https://github.com/tailwindlabs/tailwindcss/issues/19677))
3434
- Allow escape characters in `@utility` names to improve support with formatters such as Biome ([#19626](https://github.com/tailwindlabs/tailwindcss/pull/19626))
35+
- Fix invalid cache during subsequent canonicalization calls ([#19675](https://github.com/tailwindlabs/tailwindcss/pull/19675))
3536

3637
### Deprecated
3738

packages/tailwindcss/src/canonicalize-candidates.test.ts

Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1163,3 +1163,46 @@ describe('options', () => {
11631163
expect(designSystem.canonicalizeCandidates(['m-[16px]'])).toEqual(['m-[16px]']) // Ensure options don't influence shared state
11641164
})
11651165
})
1166+
1167+
// https://github.com/schoero/eslint-plugin-better-tailwindcss/issues/321
1168+
test('a subset of classes should be canonicalizable', { timeout }, async () => {
1169+
let designSystem = await designSystems.get(__dirname).get(css`
1170+
@import 'tailwindcss';
1171+
`)
1172+
1173+
let options: CanonicalizeOptions = {
1174+
collapse: true,
1175+
logicalToPhysical: true,
1176+
rem: 16,
1177+
}
1178+
1179+
expect(
1180+
designSystem.canonicalizeCandidates(['underline', 'h-4', 'w-4', 'text-sm'], options),
1181+
).toEqual(['underline', 'text-sm', 'size-4'])
1182+
})
1183+
1184+
test('collapse canonicalization is not affected by previous calls', { timeout }, async () => {
1185+
let designSystem = await designSystems.get(__dirname).get(css`
1186+
@import 'tailwindcss';
1187+
`)
1188+
1189+
let options: CanonicalizeOptions = {
1190+
collapse: true,
1191+
logicalToPhysical: true,
1192+
rem: 16,
1193+
}
1194+
1195+
let target = ['underline', 'h-4', 'w-4']
1196+
1197+
expect(designSystem.canonicalizeCandidates(target, options)).toEqual(['underline', 'size-4'])
1198+
1199+
designSystem.canonicalizeCandidates(['mb-4', 'text-sm'], options)
1200+
designSystem.canonicalizeCandidates(['underline', 'mb-4'], options)
1201+
1202+
expect(designSystem.canonicalizeCandidates(target, options)).toEqual(['underline', 'size-4'])
1203+
expect(designSystem.canonicalizeCandidates(target.concat('text-sm'), options)).toEqual([
1204+
'underline',
1205+
'text-sm',
1206+
'size-4',
1207+
])
1208+
})

packages/tailwindcss/src/canonicalize-candidates.ts

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,8 @@ function collapseCandidates(options: InternalCanonicalizeOptions, candidates: st
267267
let interestingLineHeights = new Set<string | number>()
268268
let seenLineHeights = new Set<string>()
269269
for (let pairs of candidatePropertiesValues) {
270+
if (!pairs.has('line-height')) continue
271+
270272
for (let lineHeight of pairs.get('line-height')) {
271273
if (seenLineHeights.has(lineHeight)) continue
272274
seenLineHeights.add(lineHeight)
@@ -292,6 +294,8 @@ function collapseCandidates(options: InternalCanonicalizeOptions, candidates: st
292294

293295
let seenFontSizes = new Set<string>()
294296
for (let pairs of candidatePropertiesValues) {
297+
if (!pairs.has('font-size')) continue
298+
295299
for (let fontSize of pairs.get('font-size')) {
296300
if (seenFontSizes.has(fontSize)) continue
297301
seenFontSizes.add(fontSize)

0 commit comments

Comments
 (0)