fix(canonicalize): prevent collapse cache pollution across calls#19675
fix(canonicalize): prevent collapse cache pollution across calls#19675camc314 wants to merge 1 commit intotailwindlabs:mainfrom
Conversation
WalkthroughThis pull request introduces a new 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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.
Pull request overview
Fixes an order-sensitive bug in canonicalizeCandidates where collapse-related caches could be unintentionally mutated during read-only lookups, causing later canonicalization results to depend on earlier calls (important for non-deterministic ESLint lint order).
Changes:
- Replaced
DefaultMap-backed collapse caches withMapto avoid mutation on lookup paths. - Added a
Map#getOrInsertpolyfill and refactored cache access to use explicit “insert-on-miss” behavior. - Added a regression test ensuring collapse canonicalization is stable across repeated calls.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/tailwindcss/src/utils/map-get-or-insert.ts | Introduces a Map#getOrInsert polyfill used for explicit insert-on-miss cache behavior. |
| packages/tailwindcss/src/canonicalize-candidates.ts | Refactors static-utility and utility-property caches from DefaultMap to Map, and updates collapse logic to use explicit cache population. |
| packages/tailwindcss/src/canonicalize-candidates.test.ts | Adds regression coverage for the order-sensitivity/cross-call cache pollution bug. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export {} | ||
|
|
||
| declare global { | ||
| interface Map<K, V> { | ||
| getOrInsert(key: K, defaultValue: V): V | ||
| } | ||
| } | ||
|
|
||
| if (typeof Map.prototype.getOrInsert !== 'function') { | ||
| Object.defineProperty(Map.prototype, 'getOrInsert', { | ||
| configurable: true, | ||
| enumerable: false, | ||
| writable: true, | ||
| value: function <K, V>(this: Map<K, V>, key: K, defaultValue: V): V { | ||
| if (!this.has(key)) { | ||
| this.set(key, defaultValue) | ||
| } | ||
|
|
||
| return this.get(key)! | ||
| }, | ||
| }) |
There was a problem hiding this comment.
This module both augments the global Map TypeScript interface and patches Map.prototype at runtime. Since tailwindcss is a published library, this leaks a non-standard API into consumers’ global types/runtime and can lead to conflicts with other tooling or future platform implementations. Prefer a local helper (e.g. getOrInsert(map, key, createValue)), or a small wrapper class used only for these caches, to avoid global/prototype mutation.
| export {} | |
| declare global { | |
| interface Map<K, V> { | |
| getOrInsert(key: K, defaultValue: V): V | |
| } | |
| } | |
| if (typeof Map.prototype.getOrInsert !== 'function') { | |
| Object.defineProperty(Map.prototype, 'getOrInsert', { | |
| configurable: true, | |
| enumerable: false, | |
| writable: true, | |
| value: function <K, V>(this: Map<K, V>, key: K, defaultValue: V): V { | |
| if (!this.has(key)) { | |
| this.set(key, defaultValue) | |
| } | |
| return this.get(key)! | |
| }, | |
| }) | |
| export function getOrInsert<K, V>(map: Map<K, V>, key: K, defaultValue: V): V { | |
| if (!map.has(key)) { | |
| map.set(key, defaultValue) | |
| } | |
| return map.get(key)! |
There was a problem hiding this comment.
this is a fair point, I didn't think about the fact this was a library vs runtime code.
I'll leave this up to a maintainer to decide if this should be fixed or not - please let me know and i'll change it
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 exampleh-4 + w-4 -> size-4) after unrelated prior calls.To reproduce this issue:
This should produce a failure like so:
The cause of this bug is that the canonicalization caches used
DefaultMapin places where lookups were expected to be read-only.DefaultMap.getinserts missing entried, which mutated shared cache state during intermediate lookups and made later canonicalization results depend on prior calls.By replacing the use of
DefaultMapwith a plainMap, it avoids inserting into the map on lookup paths. I've polyfilledMap#getOrInsertas it is not widely available yet, and used that where appropriate.Test plan
I wrote a test that fails on
mainbranch, I then fixed the issue, and validated that the test now passes.