mirror of
https://github.com/oxc-project/oxc.git
synced 2026-11-04 10:29:58 +00:00
fix(formatter): decide JSX opening element layout from collapsed Tailwind class string (#28744)
## Summary
With `sortTailwindcss` enabled (and `preserveWhitespace` off),
formatting a JSX element whose Tailwind class string spans multiple
lines is not idempotent. It takes two runs to reach a stable output.
Input (`{"sortTailwindcss": {}}`):
```tsx
const A = (
<div
className="p-4
flex"
>
Hello
</div>
);
```
1st run:
```tsx
const A = (
<div
className="flex p-4"
>
Hello
</div>
);
```
2nd run:
```tsx
const A = <div className="flex p-4">Hello</div>;
```
### Cause
`FormatOpeningElement` decides its layout from the attribute's
**source** string value:
- `SingleStringAttribute` is used only if the value has no line break.
- Otherwise the attributes group is forced to expand if any string value
has a line break.
But for a Tailwind attribute without `preserveWhitespace`,
`write_tailwind_string_literal` collapses all whitespace (including line
breaks) into single spaces. So the printed value has no line break while
the layout was decided as if it had one. The next run sees a single-line
value and joins the element.
prettier-plugin-tailwindcss rewrites the attribute value in the AST
before printing, so Prettier makes the layout decision on the collapsed
value and gets the 2nd-run output in one pass.
### Fix
Decide the layout from whether the **printed** value has a line break. A
Tailwind attribute (per `is_tailwind_jsx_attribute`) with
`preserveWhitespace` off is treated as single-line. Nothing changes when
`sortTailwindcss` is off, with `preserveWhitespace: true`, or for
non-Tailwind attributes.
## Test
- Added 3 cases to `apps/oxfmt/test/api/sort_tailwindcss.test.ts`:
- multiline class string in a single-attribute and a multi-attribute
element: formats in one run, and a 2nd run doesn't change the output.
Fails without this fix.
- `preserveWhitespace: true`: still keeps line breaks and expands the
element (unchanged).
- multiline non-Tailwind attribute (`title`) next to a `className`:
still expands the element (unchanged).
- `apps/oxfmt`: `vitest run` → 553 passed
- `cargo test -p oxc_formatter`, `CARGO_BUILD_WARNINGS=deny cargo lint
-p oxc_formatter`, `cargo fmt --check`, `node --run fmt` pass
- I didn't run `conformance:prettier` (fixtures not downloaded locally).
It doesn't enable `sortTailwindcss`, so this code path behaves the same
as before there.
- Found while migrating a real Next.js app to oxfmt: 42 files needed a
second `oxfmt --write` run.
---------
Co-authored-by: leaysgur <6259812+leaysgur@users.noreply.github.com>
This commit is contained in:
1 parent
2766d07f6b
commit
0b441649eb
4 files changed
+140
-49
No files matched your search
@@ -566,6 +566,100 @@ shadow-lg\` : "font-normal"}\`} />;`;
|
||||
expect(result.errors).toStrictEqual([]);
|
||||
});
|
||||
|
||||
it("should not break JSX element for multiline class string that gets collapsed", async () => {
|
||||
const input = `const A = (
|
||||
<div
|
||||
className="p-4
|
||||
flex"
|
||||
>
|
||||
Hello
|
||||
</div>
|
||||
);
|
||||
const B = (
|
||||
<div
|
||||
id="b"
|
||||
className="p-4
|
||||
flex"
|
||||
>
|
||||
Hello
|
||||
</div>
|
||||
);
|
||||
`;
|
||||
|
||||
const result = await format("test.tsx", input, {
|
||||
experimentalTailwindcss: {},
|
||||
});
|
||||
|
||||
// The value is collapsed into one line, so the element should not be expanded
|
||||
// as if the value were still multiline.
|
||||
expect(result.code).toMatchInlineSnapshot(`
|
||||
"const A = <div className="flex p-4">Hello</div>;
|
||||
const B = (
|
||||
<div id="b" className="flex p-4">
|
||||
Hello
|
||||
</div>
|
||||
);
|
||||
"
|
||||
`);
|
||||
expect(result.errors).toStrictEqual([]);
|
||||
|
||||
// Formatting the output again should not change it.
|
||||
const second = await format("test.tsx", result.code, {
|
||||
experimentalTailwindcss: {},
|
||||
});
|
||||
expect(second.code).toBe(result.code);
|
||||
});
|
||||
|
||||
it("should still break JSX element for multiline class string when preserveWhitespace is true", async () => {
|
||||
const input = `const A = (
|
||||
<div
|
||||
className="flex
|
||||
p-4"
|
||||
>
|
||||
Hello
|
||||
</div>
|
||||
);
|
||||
`;
|
||||
|
||||
const result = await format("test.tsx", input, {
|
||||
experimentalTailwindcss: { preserveWhitespace: true },
|
||||
});
|
||||
|
||||
expect(result.code).toBe(input);
|
||||
expect(result.errors).toStrictEqual([]);
|
||||
});
|
||||
|
||||
it("should still break JSX element for multiline non-Tailwind string attribute", async () => {
|
||||
const input = `const A = (
|
||||
<div
|
||||
title="first
|
||||
second"
|
||||
className="p-4 flex"
|
||||
>
|
||||
Hello
|
||||
</div>
|
||||
);
|
||||
`;
|
||||
|
||||
const result = await format("test.tsx", input, {
|
||||
experimentalTailwindcss: {},
|
||||
});
|
||||
|
||||
expect(result.code).toMatchInlineSnapshot(`
|
||||
"const A = (
|
||||
<div
|
||||
title="first
|
||||
second"
|
||||
className="flex p-4"
|
||||
>
|
||||
Hello
|
||||
</div>
|
||||
);
|
||||
"
|
||||
`);
|
||||
expect(result.errors).toStrictEqual([]);
|
||||
});
|
||||
|
||||
// Tests for can_collapse_whitespace in template literal expressions
|
||||
// These test the whitespace preservation logic based on adjacent quasi whitespace
|
||||
|
||||
|
||||
@@ -15,13 +15,12 @@ use crate::{
|
||||
ast_nodes::{AstNode, AstNodes},
|
||||
format_args,
|
||||
formatter::{
|
||||
TailwindContextEntry,
|
||||
prelude::*,
|
||||
trivia::{DanglingIndentMode, FormatDanglingComments},
|
||||
},
|
||||
utils::{
|
||||
expression::as_call_expression_without_chain_wrappers,
|
||||
tailwindcss::is_tailwind_jsx_attribute,
|
||||
tailwindcss::tailwind_context_for_jsx_attribute,
|
||||
},
|
||||
write,
|
||||
};
|
||||
@@ -300,14 +299,7 @@ impl<'a> FormatWrite<'a> for AstNode<'_, 'a, JSXAttribute<'a>> {
|
||||
write!(f, self.name());
|
||||
|
||||
if let Some(value) = &self.value() {
|
||||
// Check if this is a Tailwind attribute and push context
|
||||
// Extract context entry before mutating f
|
||||
let tailwind_ctx_to_push = f
|
||||
.options()
|
||||
.sort_tailwindcss
|
||||
.as_ref()
|
||||
.filter(|opts| is_tailwind_jsx_attribute(&self.name, opts))
|
||||
.map(|opts| TailwindContextEntry::new(opts.preserve_whitespace));
|
||||
let tailwind_ctx_to_push = tailwind_context_for_jsx_attribute(&self.name, f);
|
||||
|
||||
if let Some(ctx) = tailwind_ctx_to_push {
|
||||
f.context_mut().push_tailwind_context(ctx);
|
||||
|
||||
@@ -1,9 +1,12 @@
|
||||
use std::ops::Deref;
|
||||
|
||||
use oxc_ast::ast::{JSXAttributeItem, JSXAttributeValue, JSXOpeningElement, StringLiteral};
|
||||
use oxc_ast::ast::{JSXAttributeItem, JSXAttributeValue, JSXOpeningElement};
|
||||
use oxc_span::GetSpan;
|
||||
|
||||
use crate::{ast_nodes::AstNode, formatter::prelude::*, write};
|
||||
use crate::{
|
||||
ast_nodes::AstNode, formatter::prelude::*,
|
||||
utils::tailwindcss::tailwind_context_for_jsx_attribute, write,
|
||||
};
|
||||
|
||||
pub struct FormatOpeningElement<'a, 'b> {
|
||||
element: &'b AstNode<'b, 'a, JSXOpeningElement<'a>>,
|
||||
@@ -45,7 +48,7 @@ impl<'a, 'b> FormatOpeningElement<'a, 'b> {
|
||||
} else if attributes.len() == 1
|
||||
&& !name_has_comment
|
||||
&& !last_attribute_has_comment
|
||||
&& is_single_line_string_literal_attribute(&attributes[0])
|
||||
&& string_literal_attribute_has_line_break(&attributes[0], f) == Some(false)
|
||||
{
|
||||
OpeningElementLayout::SingleStringAttribute
|
||||
} else {
|
||||
@@ -54,17 +57,28 @@ impl<'a, 'b> FormatOpeningElement<'a, 'b> {
|
||||
}
|
||||
}
|
||||
|
||||
/// Returns `true` if this is an attribute with a [`StringLiteral`] initializer that contains at least one new line character.
|
||||
fn is_multiline_string_literal_attribute(attribute: &JSXAttributeItem<'_>) -> bool {
|
||||
/// Whether this attribute's string literal value is printed with a line break,
|
||||
/// or `None` if the value isn't a string literal.
|
||||
/// - Tailwind sorting without `preserveWhitespace` collapses line breaks,
|
||||
/// so checking the source value alone would make formatting non-idempotent
|
||||
fn string_literal_attribute_has_line_break(
|
||||
attribute: &JSXAttributeItem<'_>,
|
||||
f: &JsFormatter<'_, '_>,
|
||||
) -> Option<bool> {
|
||||
let JSXAttributeItem::Attribute(attr) = attribute else {
|
||||
return false;
|
||||
return None;
|
||||
};
|
||||
attr.value.as_ref().is_some_and(|value| {
|
||||
matches!(
|
||||
value,
|
||||
JSXAttributeValue::StringLiteral(s) if s.value.contains(|c: char| matches!(c, '\n' | '\r'))
|
||||
)
|
||||
})
|
||||
let Some(JSXAttributeValue::StringLiteral(string)) = &attr.value else {
|
||||
return None;
|
||||
};
|
||||
|
||||
if !string.value.contains(|c: char| matches!(c, '\n' | '\r')) {
|
||||
return Some(false);
|
||||
}
|
||||
|
||||
let collapses_whitespace = tailwind_context_for_jsx_attribute(&attr.name, f)
|
||||
.is_some_and(|ctx| !ctx.preserve_whitespace);
|
||||
Some(!collapses_whitespace)
|
||||
}
|
||||
|
||||
impl<'a> Format<'a, JsFormatContext<'a>> for FormatOpeningElement<'a, '_> {
|
||||
@@ -115,9 +129,9 @@ impl<'a> Format<'a, JsFormatContext<'a>> for FormatOpeningElement<'a, '_> {
|
||||
}
|
||||
});
|
||||
|
||||
let has_multiline_string_attribute = attributes
|
||||
.iter()
|
||||
.any(|attribute| is_multiline_string_literal_attribute(&attribute));
|
||||
let has_multiline_string_attribute = attributes.iter().any(|attribute| {
|
||||
string_literal_attribute_has_line_break(&attribute, f) == Some(true)
|
||||
});
|
||||
write!(f, [group(&format_inner).should_expand(has_multiline_string_attribute)]);
|
||||
}
|
||||
}
|
||||
@@ -153,26 +167,3 @@ pub enum OpeningElementLayout {
|
||||
/// ```
|
||||
IndentAttributes { name_has_comment: bool, last_attribute_has_comment: bool },
|
||||
}
|
||||
|
||||
/// Returns `true` if this is an attribute with a string literal initializer that does not contain any new line characters.
|
||||
fn is_single_line_string_literal_attribute(attribute: &JSXAttributeItem) -> bool {
|
||||
as_string_literal_attribute_value(attribute)
|
||||
.is_some_and(|string| !string.value.contains(|c: char| matches!(c, '\n' | '\r')))
|
||||
}
|
||||
|
||||
/// Returns `Some` if the initializer value of this attribute is a string literal.
|
||||
/// Returns [None] otherwise.
|
||||
fn as_string_literal_attribute_value<'a>(
|
||||
attribute: &'a JSXAttributeItem<'a>,
|
||||
) -> Option<&'a StringLiteral<'a>> {
|
||||
match attribute {
|
||||
JSXAttributeItem::Attribute(attr) => {
|
||||
if let Some(JSXAttributeValue::StringLiteral(string)) = &attr.value {
|
||||
Some(string.as_ref())
|
||||
} else {
|
||||
None
|
||||
}
|
||||
}
|
||||
JSXAttributeItem::SpreadAttribute(_) => None,
|
||||
}
|
||||
}
|
||||
@@ -50,7 +50,7 @@ pub fn tailwind_context_for_string_literal<'a>(
|
||||
/// Returns `true` for:
|
||||
/// - `class` and `className` (default attributes)
|
||||
/// - Custom attributes specified in `attributes` option
|
||||
pub fn is_tailwind_jsx_attribute(
|
||||
fn is_tailwind_jsx_attribute(
|
||||
attr_name: &JSXAttributeName<'_>,
|
||||
options: &SortTailwindcssOptions,
|
||||
) -> bool {
|
||||
@@ -68,6 +68,20 @@ pub fn is_tailwind_jsx_attribute(
|
||||
options.attributes.iter().any(|a| a == name)
|
||||
}
|
||||
|
||||
/// Returns the Tailwind context to push while printing the value of the JSX attribute `attr_name`.
|
||||
///
|
||||
/// Also read by the opening element layout, which must agree with the print on whether the value is collapsed.
|
||||
pub fn tailwind_context_for_jsx_attribute(
|
||||
attr_name: &JSXAttributeName<'_>,
|
||||
f: &JsFormatter<'_, '_>,
|
||||
) -> Option<TailwindContextEntry> {
|
||||
f.options()
|
||||
.sort_tailwindcss
|
||||
.as_ref()
|
||||
.filter(|opts| is_tailwind_jsx_attribute(attr_name, opts))
|
||||
.map(|opts| TailwindContextEntry::new(opts.preserve_whitespace))
|
||||
}
|
||||
|
||||
/// Checks if a callee expression is a Tailwind function.
|
||||
///
|
||||
/// Traverses through `CallExpression` and `MemberExpression` nodes to find the
|
||||
|
||||
Reference in new issue
Block a user