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 class extraction followed by ( in Slim #17278

Merged
merged 6 commits into from
Mar 19, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
- Remove redundant `line-height: initial` from Preflight ([#15212](https://github.com/tailwindlabs/tailwindcss/pull/15212))
- Prevent segfault when loaded in a worker thread on Linux ([#17276](https://github.com/tailwindlabs/tailwindcss/pull/17276))
- Ensure multiple `--value(…)` or `--modifier(…)` calls don't delete subsequent declarations ([#17273](https://github.com/tailwindlabs/tailwindcss/pull/17273))
- Fix class extraction followed by `(` in Slim ([#17278](https://github.com/tailwindlabs/tailwindcss/pull/17278))

## [4.0.14] - 2025-03-13

Expand Down
55 changes: 54 additions & 1 deletion crates/oxide/src/extractor/pre_processors/slim.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,23 @@ impl PreProcessor for Slim {
bracket_stack.push(cursor.curr);
}

// In slim the class name shorthand can be followed by a parenthesis. E.g.:
//
// ```slim
// body.border-t-4.p-8(attr=value)
// ^ Not part of the p-8 class
// ```
//
// This means that we need to replace all these `(` and `)` with spaces to make
// sure that we can extract the `p-8`.
//
// However, we also need to make sure that we keep the parens that are part of the
// utility class. E.g.: `bg-(--my-color)`.
b'(' if bracket_stack.is_empty() && !matches!(cursor.prev, b'-' | b'/') => {
result[cursor.pos] = b' ';
bracket_stack.push(cursor.curr);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

idk if it matters but should we worry about bg-red-500/(--my-var) here too? If so we should do !matches!(cursor.prev, b'-' | b'/')

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, answering my own question with a note: I think we can drop that matches check entirely based on the PR desc

This is because the check (after adding the /) makes these two tests I just wrote pass:

let input = r#"
    body.border-(--my-color).p-8(class="\#{body_classes}" data-hotwire-native="\#{hotwire_native_app?}" data-controller="update-time-zone")
"#;
Slim::test_extract_contains(input, vec!["border-(--my-color)", "p-8"]);

let input = r#"
    body.border-red-500/(--my-color).p-8(class="\#{body_classes}" data-hotwire-native="\#{hotwire_native_app?}" data-controller="update-time-zone")
"#;
Slim::test_extract_contains(input, vec!["border-red-500/(--my-color)", "p-8"]);

but as you mentioned this is invalid Slim syntax

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep yep, added an additional test just so that this case is covered: 1a9c242

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright we do need this for this situation: 645db11

div class="bg-(--my-color) bg-(--my-color)/(--my-opacity)"

b'(' | b'[' | b'{' => {
bracket_stack.push(cursor.curr);
}
Expand Down Expand Up @@ -116,7 +133,7 @@ mod tests {
" bg-red-500 2xl:flex bg-green-200 3xl:flex",
),
// Keep dots in strings
(r#"div(class="px-2.5")"#, r#"div(class="px-2.5")"#),
(r#"div(class="px-2.5")"#, r#"div class="px-2.5")"#),
// Replace top-level `(a-z0-9)[` with `$1 `. E.g.: `.flex[x]` -> `.flex x]`
(".text-xl.text-red-600[", " text-xl text-red-600 "),
// But keep important brackets:
Expand Down Expand Up @@ -194,6 +211,42 @@ mod tests {
Slim::test_extract_contains(input, vec!["text-red-500", "text-3xl"]);
}

// https://github.com/tailwindlabs/tailwindcss/issues/17277
#[test]
fn test_class_shorthand_followed_by_parens() {
let input = r#"
body.border-t-4.p-8(class="\#{body_classes}" data-hotwire-native="\#{hotwire_native_app?}" data-controller="update-time-zone")
"#;
Slim::test_extract_contains(input, vec!["border-t-4", "p-8"]);

// Additional test with CSS Variable shorthand syntax in the attribute itself because `(`
// and `)` are not valid in the class shorthand version.
//
// Also included an arbitrary value including `(` and `)` to make sure that we don't
// accidentally remove those either.
let input = r#"
body.p-8(class="bg-(--my-color) bg-(--my-color)/(--my-opacity) bg-[url(https://example.com)]")
"#;
Slim::test_extract_contains(
input,
vec![
"p-8",
"bg-(--my-color)",
"bg-(--my-color)/(--my-opacity)",
"bg-[url(https://example.com)]",
],
);

// Top-level class shorthand with parens
let input = r#"
div class="bg-(--my-color) bg-(--my-color)/(--my-opacity)"
"#;
Slim::test_extract_contains(
input,
vec!["bg-(--my-color)", "bg-(--my-color)/(--my-opacity)"],
);
}

#[test]
fn test_strings_only_occur_when_nested() {
let input = r#"
Expand Down