Allow multiple @utility definitions with same name but different value types#19777
Conversation
|
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 (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThe PR changes 🚥 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. Review rate limit: 6/8 reviews remaining, refill in 10 minutes and 36 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tailwindcss/src/compile.ts (1)
309-312: Consider adding the explanatory comment here for consistency.The fallback utilities loop applies identical null-handling logic but lacks the explanatory comment present in lines 291-294. Adding a brief comment would improve maintainability and make the parallel behavior explicit.
📝 Suggested improvement
if (compiledNodes === null) { + // Same typed vs untyped distinction as above if (utility.options?.types?.length) return asts continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tailwindcss/src/compile.ts` around lines 309 - 312, Add the same explanatory comment that exists earlier for null-handling to the fallback utilities loop where compiledNodes is checked (the block: if (compiledNodes === null) { if (utility.options?.types?.length) return asts continue }). Locate the fallback utilities loop in compile.ts and insert a brief comment above that if-statement explaining why compiledNodes can be null and why we return asts when utility.options?.types?.length is set (mirroring the comment used for the earlier, identical null-handling logic), so the parallel behavior is explicit and maintainable.
🤖 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/compile.ts`:
- Around line 309-312: Add the same explanatory comment that exists earlier for
null-handling to the fallback utilities loop where compiledNodes is checked (the
block: if (compiledNodes === null) { if (utility.options?.types?.length) return
asts continue }). Locate the fallback utilities loop in compile.ts and insert a
brief comment above that if-statement explaining why compiledNodes can be null
and why we return asts when utility.options?.types?.length is set (mirroring the
comment used for the earlier, identical null-handling logic), so the parallel
behavior is explicit and maintainable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c7b0a62d-f8cb-4ef4-817e-22d1599afd98
📒 Files selected for processing (2)
packages/tailwindcss/src/compile.tspackages/tailwindcss/src/utilities.test.ts
…different value types When multiple CSS @Utility definitions share the same name but handle different value types, all handlers should be tried. Previously, a null return from any handler would stop the loop, preventing subsequent handlers from being attempted. The fix distinguishes between CSS @Utility handlers (no typed options) and JS plugin matchUtilities handlers (with explicit types). For CSS @Utility, null means "try the next handler." For typed plugin utilities, null means "the value was invalid for this type, stop." Fixes tailwindlabs#16948 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
RobinMalfait
force-pushed
the
osc/16948-fix-utility-same-name-conflicts
branch
from
c8f752f to
055e203
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tailwindcss/src/compile.ts (1)
290-297: Consider extracting duplicated null-handling to one helper.Both loops now carry identical branching for
null; centralizing it will reduce future drift risk.♻️ Proposed refactor
+function shouldStopOnNull(utility: Utility) { + return Boolean(utility.options?.types?.length) +} + function compileBaseUtility(candidate: Candidate, designSystem: DesignSystem) { @@ let compiledNodes = utility.compileFn(candidate) if (compiledNodes === undefined) continue if (compiledNodes === null) { - // For typed plugin utilities (matchUtilities with explicit types), - // null means the value was invalid for this type - stop trying. - // For CSS `@utility` definitions (no types), null means the value - // didn't match this handler - try the next one. - if (utility.options?.types?.length) return asts + if (shouldStopOnNull(utility)) return asts continue } @@ let compiledNodes = utility.compileFn(candidate) if (compiledNodes === undefined) continue if (compiledNodes === null) { - if (utility.options?.types?.length) return asts + if (shouldStopOnNull(utility)) return asts continue }Also applies to: 309-312
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tailwindcss/src/compile.ts` around lines 290 - 297, There are two identical null-handling branches after computing compiledNodes; extract that logic into a helper (e.g., a function like handleCompiledNodesNull(compiledNodes, utilityOptions, asts)) that returns a sentinel (boolean or special value) indicating whether to continue the outer loop or return asts, and replace both duplicated if (compiledNodes === null) blocks with a single call to this helper; ensure the helper checks utility.options?.types?.length to decide between returning asts or continuing so behavior of the loops (the places that reference compiledNodes, utility.options?.types?.length and asts) remains unchanged.
🤖 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/compile.ts`:
- Around line 290-297: There are two identical null-handling branches after
computing compiledNodes; extract that logic into a helper (e.g., a function like
handleCompiledNodesNull(compiledNodes, utilityOptions, asts)) that returns a
sentinel (boolean or special value) indicating whether to continue the outer
loop or return asts, and replace both duplicated if (compiledNodes === null)
blocks with a single call to this helper; ensure the helper checks
utility.options?.types?.length to decide between returning asts or continuing so
behavior of the loops (the places that reference compiledNodes,
utility.options?.types?.length and asts) remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b06e5db7-4187-4c11-9deb-e77f1996429e
📒 Files selected for processing (2)
packages/tailwindcss/src/compile.tspackages/tailwindcss/src/utilities.test.ts
✅ Files skipped from review due to trivial changes (1)
- packages/tailwindcss/src/utilities.test.ts
There was a problem hiding this comment.
Thanks! Did additional testing related to legacy matchUtilities behavior and I think the current implementation is fine.
We could also allow matchUtilities that have any as a type to continue, but that could result in more (unwanted) changes in current projects. We could also not care about the types, and just ignore the results of the current plugin when we return null (just like we do in @utility) but that would also probably cause unwanted changes in current projects.
2 participants
Summary
Fixes #16948
When defining multiple CSS
@utility foo-*with different value types (e.g., one for colors, one for numbers), only the first handler was tried. If it returnednull(value didn't match), the compile loop stopped, preventing subsequent handlers from being attempted.Previously,
foo-red-500worked butfoo-123did not (or vice versa depending on definition order).The fix distinguishes between CSS
@utilityhandlers and JS pluginmatchUtilitieshandlers:@utility(no typed options):nullmeans "try the next handler" - allows multiple definitions with different value types to coexistmatchUtilities(with explicit types):nullmeans "the value was invalid for this type, stop" - preserves existing behavior where typed utilities prevent invalid values from falling throughTest plan
@utility foo-*definitions with different value types - verifies bothfoo-red-500(color) andfoo-123(number) produce correct CSSmatchUtilitiestype-safety tests)pnpm build && pnpm testpassesThis contribution was developed with AI assistance (Claude Code).