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

Do not extract candidates containing JS string interpolation pattern ${ #17142

Merged
merged 3 commits into from
Mar 12, 2025

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Mar 12, 2025

This PR fixes an issue where often people run into issues where they try to use string interpolation and it doesn't work. Even worse, it could result in crashes because we will actually generate CSS. This fix only filters out candidates with a pattern like ${. If this occurs in a string position it is fine.

Another solution would be to add a pre processor for JS/TS (and all thousand file extension combinations) but the problem is that you can also write JS in HTML files so we would have to pre process HTML as well which would not be ideal.

Test plan

  1. Added tests to prove this works in arbitrary values, arbitrary variables in both utilities and variants.
  2. Existing tests pass.
  3. Some screenshots with before / after situations:

Given this input:

let color = '#0088cc';
let opacity = 0.8;
let name = 'variable-name';
let classes = [
  // Arbitrary Properties
  `[color:${color}]`
  `[${property}:value]`,
  `[--img:url('https://example.com?q=${name}')]`, // WONT WORK BUT VALID CSS

  // Arbitrary Values
  `bg-[${color}]`,

  // Arbitrary Variables
  `bg-(--my-${color})`,
  `bg-(--my-color,${color})`,

  // Arbitrary Modifier
  `bg-red-500/[${opacity}]`,
  `bg-red-500/(--my-${name})`,
  `bg-red-500/(--my-opacity,${opacity})`,

  // Arbitrary Variant
  `data-[state=${name}]:flex`,
  `supports-(--my-${name}):flex`,
  `[@media(width>=${value})]:flex`,
];

This is the result:

Before After
image image

Fixes: #17054
Fixes: #15853

If this occurs inside of strings then it's fine but everywhere else it's
thrown out.

Technically you can set it in the value of a CSS variable, but I think
guarding against that is only needed if somebody actually runs into
problems with this.
@RobinMalfait RobinMalfait marked this pull request as ready for review March 12, 2025 11:04
@RobinMalfait RobinMalfait requested a review from a team as a code owner March 12, 2025 11:04
Copy link
Member

@philipp-spiess philipp-spiess left a comment

Choose a reason for hiding this comment

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

Nice one!

@philipp-spiess
Copy link
Member

@RobinMalfait Here's another issue that is resolved by this I think: #15853

@RobinMalfait RobinMalfait merged commit ca408d0 into main Mar 12, 2025
5 checks passed
@RobinMalfait RobinMalfait deleted the fix/issue-17054 branch March 12, 2025 11:09
ilievmart added a commit to 42dotmk/KlimentOhridski that referenced this pull request Mar 20, 2025
This also fixes <ol> and <ul> tag rendering which broke in today's dependency update.

Related: tailwindlabs/tailwindcss#17142
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants