Don't escape underscores for the first parameter of var() #14776
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Member Author
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @philipp-spiess and the rest of your teammates on |
philipp-spiess force-pushed the 10-24-don_t_escape_underscores_for_the_first_parameter_of_var_ branch from 8879dbb to 951e6a6 Compare 
philipp-spiess commented Oct 24, 2024
Comment on lines +69 to +70
| 'calc(var(--10-10px,calc(-20px-(-30px--40px)-50px)))', | ||
| 'calc(var(--10-10px,calc(-20px - (-30px - -40px) - 50px)))', |
Member Author
There was a problem hiding this comment.
This test case didn't have the proper number of closing quotes. I remember that @RobinMalfait once mentioned that we do validate this already at the parser level.
Member
There was a problem hiding this comment.
I think we only validate the parens in your CSS file (and throw), but we don't validate if it happens in a candidate.
Input:
<div class="[--foo:calc(var(--10-10px,calc(-20px-(-30px--40px)-50px)]"></div>Output:
.\[--foo\:calc\(var\(--10-10px\,calc\(-20px-\(-30px--40px\)-50px\)\] { --foo: calc(var(--10-10px,calc(-20px - (-30px - -40px) - 50px); } Member Author
There was a problem hiding this comment.
I see, lol!
With this change the closing parentheses would be automatically inserted actually (because we can't encode the absence of them in the ValueParser—That's how I found out about this example in the first place! But I think it would be better if we throw away candidates like this 🤔
philipp-spiess commented Oct 24, 2024
Comment on lines +6 to 8
| if (input.indexOf('(') === -1) { | ||
| return convertUnderscoresToWhitespace(input) | ||
| } |
Member Author
There was a problem hiding this comment.
This check was moved up from being inside the addWhitespaceAroundMathOperators, since neither the url() nor the var() handling need to run if the arbitrary value has no parenthesis.
philipp-spiess force-pushed the 10-24-don_t_escape_underscores_for_the_first_parameter_of_var_ branch from 951e6a6 to 1b1d7b9 Compare 
adamwathan reviewed Oct 24, 2024
philipp-spiess force-pushed the 10-24-don_t_escape_underscores_for_the_first_parameter_of_var_ branch from 6460bcb to 21eb011 Compare 
Co-authored-by: Adam Wathan <adam.wathan@gmail.com>
philipp-spiess force-pushed the 10-24-don_t_escape_underscores_for_the_first_parameter_of_var_ branch from 21eb011 to 75aa966 Compare adamwathan approved these changes Oct 24, 2024
adamwathan deleted the 10-24-don_t_escape_underscores_for_the_first_parameter_of_var_ branch adamwathan pushed a commit that referenced this pull request Oct 24, 2024
Quick follow-up to #14776 to treat the `theme()` function the same way.
adamwathan added a commit that referenced this pull request Oct 24, 2024
This PR fixes an issue where currently a `theme()` function call inside an arbitrary value that used a dot in the key path: ```jsx let className = "ml-[theme(spacing[1.5])]" ``` Was causing issues when going though the codemod. The issue is that for candidates, we require `_` to be _escaped_, since otherwise they will be replaced with underscore. When going through the codemods, the above candidate would be translated to the following CSS variable access: ```js let className = "ml-[var(--spacing-1\_5))" ``` Because the underscore was escaped, we now have an invalid string inside a JavaScript file (as the `\` would escape inside the quoted string. To resolve this, we decided that this common case (as its used by the Tailwind CSS default theme) should work without escaping. In #14776, we made the changes that CSS variables used via `var()` no longer unescape underscores. This PR extends that so that the Variant printer (that creates the serialized candidate representation after the codemods make changes) take this new encoding into account. This will result in the above example being translated into: ```js let className = "ml-[var(--spacing-1_5))" ``` With no more escaping. Nice! ## Test Plan I have added test for this to the kitchen-sink upgrade tests. Furthermore, to ensure this really works full-stack, I have updated the kitchen-sink test to _actually build the migrated project with Tailwind CSS v4_. After doing so, we can assert that we indeed have the right class name in the generated CSS. --------- Co-authored-by: Adam Wathan <adam.wathan@gmail.com>
philipp-spiess added a commit that referenced this pull request Feb 4, 2025
Resolves #16170 This PR fixes an issue where the previously opted-out escaping of the first argument for the `var(…)` function was not unescaped at all. This was introduced in #14776 where the intention was to not require escaping of underscores in the var function (e.g. `ml-[var(--spacing-1_5)]`). However, I do think it still makes sense to unescape an eventually escaped underline for consistency. ## Test plan The example from #1670 now parses as expected: <img width="904" alt="Screenshot 2025-02-03 at 13 51 35" src="https://github.com/user-attachments/assets/cac0f06e-37da-4dcb-a554-9606d144a8d5" /> --------- Co-authored-by: Robin Malfait <malfait.robin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Add this suggestion to a batch that can be applied as a single commit. This suggestion is invalid because no changes were made to the code. Suggestions cannot be applied while the pull request is closed. Suggestions cannot be applied while viewing a subset of changes. Only one suggestion per line can be applied in a batch. Add this suggestion to a batch that can be applied as a single commit. Applying suggestions on deleted lines is not supported. You must change the existing code in this line in order to create a valid suggestion. Outdated suggestions cannot be applied. This suggestion has been applied or marked resolved. Suggestions cannot be applied from pending reviews. Suggestions cannot be applied on multi-line comments. Suggestions cannot be applied while the pull request is queued to merge. Suggestion cannot be applied right now. Please check back later.
This PR updates our arbitrary value decoder to:
No longer require an escaping for underscores in the first parameter of
var(). Example:Ensures that properties before an eventual
url()are properly unescaped. Example:I will ensure that this properly works for the migrate use case in a follow-up PR in the stack.
Test Plan
Added unit tests as well as tests for the variant decoder. Additionally this PR also adds a higher-level test using the public Tailwind APIs to ensure this is properly propagated.