Skip to content

Commit

Permalink
fix(rome_js_formatter): in JSX, some spaces are removed rome#3211
Browse files Browse the repository at this point in the history
  • Loading branch information
@denbezrukov
denbezrukov committed Sep 19, 2022
1 parent 35d293f commit d03f026
Show file tree
Hide file tree
Showing 5 changed files with 329 additions and 27 deletions.
12 changes: 9 additions & 3 deletions crates/rome_js_formatter/src/jsx/lists/child_list.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use crate::utils::jsx::{
use crate::JsFormatter;
use rome_formatter::{format_args, write, FormatRuleWithOptions, VecBuffer};
use rome_js_syntax::{JsxAnyChild, JsxChildList};
use rome_rowan::TextSize;

#[derive(Debug, Clone, Default)]
pub struct FormatJsxChildList {
Expand Down Expand Up @@ -142,7 +143,7 @@ impl FormatRule<JsxChildList> for FormatJsxChildList {
// Any child that isn't text
JsxChild::NonText(non_text) => {
let line_mode = match children_iter.peek() {
Some(JsxChild::Newline | JsxChild::Word(_) | JsxChild::Whitespace) => {
Some(JsxChild::Word(word)) => {
// Break if the current or next element is a self closing element
// ```javascript
// <pre className="h-screen overflow-y-scroll" />adefg
Expand All @@ -152,16 +153,21 @@ impl FormatRule<JsxChildList> for FormatJsxChildList {
// <pre className="h-screen overflow-y-scroll" />
// adefg
// ```
if matches!(non_text, JsxAnyChild::JsxSelfClosingElement(_)) {
if matches!(non_text, JsxAnyChild::JsxSelfClosingElement(_))
&& word.len() != TextSize::from(1)
{
Some(LineMode::Hard)
} else {
Some(LineMode::Soft)
}
}

// Add a hard line break if what comes after the element is not a text or is all whitespace
Some(_) => Some(LineMode::Hard),
Some(JsxChild::NonText(_)) => Some(LineMode::Hard),

Some(JsxChild::Newline | JsxChild::Whitespace | JsxChild::EmptyLine) => {
None
}
// Don't insert trailing line breaks
None => None,
};
Expand Down
3 changes: 2 additions & 1 deletion crates/rome_js_formatter/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -750,7 +750,8 @@ function() {
// use this test check if your snippet prints as you wish, without using a snapshot
fn quick_test() {
let src = r#"
type C = B & (C | A) & B;
const a = <><b/>a{' '}{" "}{" "}{" "}c{" "}{" "}{" "}{" "}{" "}{" "}</>;
"#;
let syntax = SourceType::tsx();
Expand Down
118 changes: 100 additions & 18 deletions crates/rome_js_formatter/src/utils/jsx.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ impl Format<JsFormatContext> for JsxSpace {
write![
formatter,
[
if_group_breaks(&format_args![JsxRawSpace, hard_line_break()]),
if_group_breaks(&format_args![JsxRawSpace, soft_line_break()]),
if_group_fits_on_line(&space())
]
]
Expand Down Expand Up @@ -173,7 +173,7 @@ pub(crate) fn jsx_split_children<I>(children: I) -> SyntaxResult<Vec<JsxChild>>
where
I: IntoIterator<Item = JsxAnyChild>,
{
let mut result = Vec::new();
let mut builder = JsxSplitChildrenBuilder::default();

for child in children.into_iter() {
match child {
Expand Down Expand Up @@ -203,15 +203,15 @@ where
// </div>
// ```
if newlines > 1 {
result.push(JsxChild::EmptyLine);
builder.entry(JsxChild::EmptyLine);
}

continue;
}

result.push(JsxChild::Newline)
} else if !matches!(result.last(), Some(JsxChild::Whitespace)) {
result.push(JsxChild::Whitespace)
builder.entry(JsxChild::Newline)
} else {
builder.entry(JsxChild::Whitespace)
}
}
_ => unreachable!(),
Expand All @@ -224,15 +224,15 @@ where
// Only handle trailing whitespace. Words must always be joined by new lines
if chunks.peek().is_none() {
if whitespace.contains('\n') {
result.push(JsxChild::Newline);
builder.entry(JsxChild::Newline);
} else {
result.push(JsxChild::Whitespace)
builder.entry(JsxChild::Whitespace)
}
}
}

(relative_start, JsxTextChunk::Word(word)) => {
result.push(JsxChild::Word(JsxWord {
builder.entry(JsxChild::Word(JsxWord {
text: value_token
.token_text()
.slice(TextRange::at(relative_start, word.text_len())),
Expand All @@ -245,23 +245,61 @@ where

JsxAnyChild::JsxExpressionChild(child) => {
if is_whitespace_jsx_expression(&child) {
match result.last() {
Some(JsxChild::Whitespace) => {
// Ignore
}
_ => result.push(JsxChild::Whitespace),
}
builder.entry(JsxChild::Whitespace)
} else {
result.push(JsxChild::NonText(child.into()))
builder.entry(JsxChild::NonText(child.into()))
}
}
child => {
result.push(JsxChild::NonText(child));
builder.entry(JsxChild::NonText(child));
}
}
}

Ok(builder.finish())
}

#[derive(Debug, Default)]
struct JsxSplitChildrenBuilder {
pending: Option<JsxChild>,
buffer: Vec<JsxChild>,
}

impl JsxSplitChildrenBuilder {
fn is_last_whitespace(&self) -> bool {
matches!(self.buffer.last(), Some(JsxChild::Whitespace))
}

fn entry(&mut self, child: JsxChild) {
if let Some(pending) = self.pending.take() {
if matches!(
pending,
JsxChild::EmptyLine | JsxChild::Newline | JsxChild::Whitespace
) {
if !matches!(child, JsxChild::Whitespace) && !self.is_last_whitespace() {
self.buffer.push(pending)
}
} else {
self.buffer.push(pending);
}
}

self.pending = Some(child);
}

Ok(result)
fn finish(mut self) -> Vec<JsxChild> {
if let Some(pending) = self.pending.take() {
if matches!(pending, JsxChild::Whitespace) {
if !self.is_last_whitespace() {
self.buffer.push(pending)
}
} else {
self.buffer.push(pending);
}
}

self.buffer
}
}

#[derive(Debug, Clone, Eq, PartialEq)]
Expand Down Expand Up @@ -328,6 +366,12 @@ pub(crate) struct JsxWord {
source_position: TextSize,
}

impl JsxWord {
pub fn len(&self) -> TextSize {
self.text.len()
}
}

impl Format<JsFormatContext> for JsxWord {
fn fmt(&self, f: &mut Formatter<JsFormatContext>) -> FormatResult<()> {
f.write_element(FormatElement::Text(Text::SyntaxTokenTextSlice {
Expand Down Expand Up @@ -585,6 +629,44 @@ mod tests {
assert_eq!(children[3], JsxChild::Whitespace);
}

#[test]
fn split_children_remove_in_row_jsx_whitespaces() {
let child_list = parse_jsx_children(r#"a{' '}{' '}{' '}c{" "}{' '}{" "}"#);

let children = jsx_split_children(&child_list).unwrap();

assert_eq!(
4,
children.len(),
"Expected to contain four elements. Actual:\n{children:#?} "
);
assert_word(&children[0], "a");
assert_eq!(children[1], JsxChild::Whitespace);
assert_word(&children[2], "c");
assert_eq!(children[3], JsxChild::Whitespace);
}

#[test]
fn split_children_remove_new_line_before_jsx_whitespaces() {
let child_list = parse_jsx_children(
r#"a
{' '}c{" "}
"#,
);

let children = jsx_split_children(&child_list).unwrap();

assert_eq!(
4,
children.len(),
"Expected to contain four elements. Actual:\n{children:#?} "
);
assert_word(&children[0], "a");
assert_eq!(children[1], JsxChild::Whitespace);
assert_word(&children[2], "c");
assert_eq!(children[3], JsxChild::Whitespace);
}

fn assert_word(child: &JsxChild, text: &str) {
match child {
JsxChild::Word(word) => {
Expand Down
69 changes: 69 additions & 0 deletions crates/rome_js_formatter/tests/specs/jsx/element.jsx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,75 @@ c = <div>a{' '}{' '}{' '}{' '}{' '}{' '}{' '}{' '}b{' '}{' '}{' '}{' '}{' '}{' '

c2 = <div>a{' '}{' '}{' '}{' '}{' '}{' '}{' '}{' '}<div></div>content{' '}{' '}{' '}{' '}{' '}{' '}</div>;

// this group should fit one line jsx whitespaces are hidden
b =
<div>
<a></a>

{' '}

<a></a>

{' '}
1
</div>;

// this group should break first tag and show only first jsx whitespace
b1 =
<div>
<a>
{`
12312
12312
`}
</a>

{' '}

<a></a>

{' '}
1
</div>;

// this group fit one line and hide jsx whitespace
b2 =
<>
<a>123
</a>

{' '}
1
</>;

// this group break group and show jsx whitespace
b3 =
<>
<a>{`
123`}
</a>

{' '}
1
</>;

// one length text should insert soft line break
b4 = <><pre />a</>;
// longer than one length text should insert hard line break
b5 = <><pre />ab</>;

// one length text should insert soft line break show last jsx whitespace
b6 = <><b/>a{' '}bb{" "}cc{" "}dd{" "}ee{" "}{" "}{" "}{" "}{" "}{" "}</>;
// longer than one length text should insert hard line break
b7 = <><b/>longer{' '}bb{" "}cc{" "}dd{" "}ee{" "}{" "}{" "}{" "}{" "}{" "}</>;

const b8 = <div>
Text <a data-very-long-prop-breakline-rome-playground data-other>
some link
</a>{' '}
| some other text,{' '}
</div>;

<div><div></div><a> jumps over the lazy dog </a></div>;

const Essay = () => <div>The films of Wong Kar-Wai exemplify the synthesis of French New Wave cinema—specifically the unrelenting
Expand Down
Loading

0 comments on commit d03f026

Please sign in to comment.