Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix CSS theme() function resolution issue #14614

Merged
merged 6 commits into from
Oct 10, 2024

Conversation

philipp-spiess
Copy link
Member

@philipp-spiess philipp-spiess commented Oct 7, 2024

We currently have three different implementations of the resolveThemeValue() Theme method:

  1. The core version which only resolves based on the exact CSS variable name. This is the v4-only version.
  2. A compat light version which attempts to translate a dot-notation keypath to a CSS variable name.
  3. A full compat version which resolves the legacy theme object which is used whenever a @plugin or @config is added.

Unfortunately, there are some issues with this approach that we've encountered when testing the CSS theme() function while upgrading Tailwind Templates to v4 using our upgrading toolkit.

  1. The compat light resolver was trying to convert theme(spacing.1) to tuple syntax. Tuples require a nested property access, though, and instead this should be convert to theme(--spacing-1).
  2. We currently only load full compat version if @plugin or @config directives are used. It is possible, though, that we need the full compat mapping in other cases as well. One example we encountered is theme(fontWeight.semibold) which maps to a dictionary in the default theme that that we do not have an equivalent for in v4 variables.

To fix both issues, we decided to remove the compat light version of the theme() function in favor of adding this behavior to an upgrade codemod. Instead, the second layer now only decides wether it can use the core version or wether it needs to upgrade to the full compat version. Since typos would trigger a hard error, we do not think this has unintended performance consequences if someone wants to use the core version only (they would get an error regardless which they need to fix in order to continue).

@philipp-spiess philipp-spiess marked this pull request as ready for review October 8, 2024 10:01
@philipp-spiess philipp-spiess force-pushed the fix/theme-function-resolution branch from 03b956d to 0332596 Compare October 9, 2024 12:33
@philipp-spiess philipp-spiess force-pushed the fix/theme-function-resolution branch from 0332596 to f6ba774 Compare October 9, 2024 12:58
Copy link
Contributor

@thecrypticace thecrypticace left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see comment about upgradeToFullPluginSupport

@philipp-spiess philipp-spiess merged commit dc69802 into next Oct 10, 2024
1 check passed
@philipp-spiess philipp-spiess deleted the fix/theme-function-resolution branch October 10, 2024 10:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants