Make createCompatConfig theme mappings safe to spread when a namespace returns a string - #20399
Conversation
…ace returns a string
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Reviews (3): Last reviewed commit: "update changelog" | Re-trigger Greptile |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughCompatibility theme lookups now preserve object results for namespace roots while unwrapping synthetic 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
|
What's a situation/use case where you ran into this kind of issue? |
I ran into this while working on Jumi, a Tailwind CSS v4 animation plugin I maintain. I was resolving a project's theme through the compat layer so Jumi could read the animation values, and for certain themes the output just looked… wrong. A value that should come back as What made me think this could've gone unnoticed for a while is that it's completely silent. The config still resolves, so one would only notice when something (typically, a plugin) actually reads those theme values. I also had several cases of arbitrary class values returning as just |
…ject
In Tailwind CSS v3, each namespace inside the `theme` configuration has
to be an object. If you want to use `animate` instead of `animate-foo`,
then you have to define this as
```js
module.exports = {
theme: {
animate: {
DEFAULT: '…'
}
}
}
```
If we then use `theme('animate')` in our JS config/plugin, then we
must receive the entire object, not the default value.
Additionally, in Tailwind CSS v4, if you use:
```css
@theme {
--animate: …;
}
```
If you then use `theme('animate')` you also have to get the object with
the `DEFAULT` key in there.
If we don't do this, then places where we assume that it's an object
would break. E.g.:
```js
module.exports = {
theme: {
animation: ({ theme }) => ({
...theme('animate', {}),
}),
}
}
```
Luckily, even in the Tailwind CSS v3 days this returned an object. So if
you do want the value, you can use `theme('animate.DEFAULT')`. Which
also works today in both Tailwind CSS v3 and v4.
Side note, this is only for the `theme` function exposed to the JS based
plugins and config files. If you use `theme(animate)` as part of your
CSS, you will get the DEFAULT value, because you can't use objects in
your CSS anyway.
Yes and no, you would notice it given that some things just don't work if you do this. If you don't notice anything wrong, it means you're not using anything related to this and nothing will be broken. Overal, I think the idea is good, but I don't like the This is how Tailwind CSS v3 behaves where a top-level namespace like this should result in an object. There is a chance that returning an object instead of a string will break, but in that case the fix in user land would be to use I pushed some changes to this branch. |
RobinMalfait
left a comment
RobinMalfait
left a comment
There was a problem hiding this comment.
I think this should do it, thanks for the PR!
2 participants
When building the legacy config compat layer, namespace mappings like
fontSize→text,boxShadow→shadow,animation→animate, etc. spread the result oftheme(namespace, {})directly:Some of these namespaces can return a string (e.g.
theme('text', {})→'1rem'). Spreading a string produces char-indexed keys ({ '0': '1', '1': 'r', ... }) instead of a{ DEFAULT: ... }entry, silently corrupting the compat config output.This PR adds a small
spreadThemehelper that normalizes the return value before spreading:string→{ DEFAULT: value }{ ...value }{}and updates all namespace mappings to use it. Includes unit tests for the helper plus integration tests for the affected namespaces and the string-return regression cases.