Skip to content
This repository has been archived by the owner on Aug 31, 2023. It is now read-only.

Commit

Permalink
fix(rome_js_formatter): in JSX, some spaces are removed #3211
Browse files Browse the repository at this point in the history
  • Loading branch information
@denbezrukov
denbezrukov committed Oct 10, 2022
1 parent c876290 commit 5831a07
Show file tree
Hide file tree
Showing 8 changed files with 407 additions and 912 deletions.
87 changes: 64 additions & 23 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 @@ -108,17 +108,22 @@ impl FormatJsxChildList {
),
}),

_ => None,
Some(JsxChild::Newline | JsxChild::Whitespace | JsxChild::EmptyLine) => {
None
}

None => None,
};

child_breaks = separator.map_or(false, |separator| separator.will_break());

flat.write(&format_args![word, separator], f);

if let Some(separator) = separator {
multiline.write(word, &separator, f);
multiline.write_with_separator(word, &separator, f);
} else {
multiline.write_with_empty_separator(word, f);
// it's safe to write without a separator because None means that next element is a separator or end of the iterator
multiline.write_content(word, f);
}
}

Expand All @@ -139,7 +144,7 @@ impl FormatJsxChildList {
let is_trailing_or_only_whitespace = children_iter.peek().is_none();

if is_trailing_or_only_whitespace || is_after_line_break {
multiline.write_with_empty_separator(&JsxRawSpace, f);
multiline.write_separator(&JsxRawSpace, f);
}
// Leading whitespace. Only possible if used together with a expression child
//
Expand All @@ -151,30 +156,30 @@ impl FormatJsxChildList {
// </div>
// ```
else if last.is_none() {
multiline.write(&JsxRawSpace, &hard_line_break(), f);
multiline.write_with_separator(&JsxRawSpace, &hard_line_break(), f);
} else {
multiline.write_with_empty_separator(&JsxSpace, f);
multiline.write_separator(&JsxSpace, f);
}
}

// A new line between some JSX text and an element
JsxChild::Newline => {
child_breaks = true;

multiline.write_with_empty_separator(&hard_line_break(), f);
multiline.write_separator(&hard_line_break(), f);
}

// An empty line between some JSX text and an element
JsxChild::EmptyLine => {
child_breaks = true;

multiline.write_with_empty_separator(&empty_line(), f);
multiline.write_separator(&empty_line(), f);
}

// 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 @@ -184,36 +189,54 @@ impl FormatJsxChildList {
// <pre className="h-screen overflow-y-scroll" />
// adefg
// ```
if matches!(non_text, JsxAnyChild::JsxSelfClosingElement(_)) {
if matches!(non_text, JsxAnyChild::JsxSelfClosingElement(_))
&& !word.is_ascii_punctuation()
{
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,
};

child_breaks = line_mode.map_or(false, |mode| mode.is_hard());

let format_separator = format_with(|f| match line_mode {
Some(mode) => f.write_element(FormatElement::Line(mode)),
None => Ok(()),
let format_separator = line_mode.map(|mode| {
format_with(move |f| f.write_element(FormatElement::Line(mode)))
});

if force_multiline {
multiline.write(&non_text.format(), &format_separator, f);
if let Some(format_separator) = format_separator {
multiline.write_with_separator(
&non_text.format(),
&format_separator,
f,
);
} else {
// it's safe to write without a separator because None means that next element is a separator or end of the iterator
multiline.write_content(&non_text.format(), f);
}
} else {
let mut memoized = non_text.format().memoized();

force_multiline = memoized.inspect(f)?.will_break();

flat.write(&format_args![memoized, format_separator], f);
multiline.write(&memoized, &format_separator, f);

if let Some(format_separator) = format_separator {
multiline.write_with_separator(&memoized, &format_separator, f);
} else {
// it's safe to write without a separator because None means that next element is a separator or end of the iterator
multiline.write_content(&memoized, f);
}
}
}
}
Expand Down Expand Up @@ -476,18 +499,30 @@ impl MultilineBuilder {
}

/// Formats an element that does not require a separator
fn write_with_empty_separator(
/// It is safe to omit the separator because at the call side we must guarantee that we have reached the end of the iterator
/// or the next element is a space/newline that should be written into the separator "slot".
fn write_content(&mut self, content: &dyn Format<JsFormatContext>, f: &mut JsFormatter) {
self.write(content, None, f);
}

/// Formatting a separator does not require any element in the separator slot
fn write_separator(&mut self, separator: &dyn Format<JsFormatContext>, f: &mut JsFormatter) {
self.write(separator, None, f);
}

fn write_with_separator(
&mut self,
content: &dyn Format<JsFormatContext>,
separator: &dyn Format<JsFormatContext>,
f: &mut JsFormatter,
) {
self.write(content, &format_with(|_| Ok(())), f)
self.write(content, Some(separator), f);
}

fn write(
&mut self,
content: &dyn Format<JsFormatContext>,
separator: &dyn Format<JsFormatContext>,
separator: Option<&dyn Format<JsFormatContext>>,
f: &mut JsFormatter,
) {
let result = std::mem::replace(&mut self.result, Ok(Vec::new()));
Expand All @@ -502,12 +537,18 @@ impl MultilineBuilder {
write!(buffer, [content])?;
buffer.write_element(FormatElement::Tag(Tag::EndEntry))?;

buffer.write_element(FormatElement::Tag(Tag::StartEntry))?;
write!(buffer, [separator])?;
buffer.write_element(FormatElement::Tag(Tag::EndEntry))?;
if let Some(separator) = separator {
buffer.write_element(FormatElement::Tag(Tag::StartEntry))?;
write!(buffer, [separator])?;
buffer.write_element(FormatElement::Tag(Tag::EndEntry))?;
}
}
MultilineLayout::NoFill => {
write!(buffer, [content, separator])?;

if let Some(separator) = separator {
write!(buffer, [separator])?;
}
}
};
buffer.into_vec()
Expand Down
13 changes: 10 additions & 3 deletions crates/rome_js_formatter/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -861,9 +861,16 @@ function() {
// use this test check if your snippet prints as you wish, without using a snapshot
fn quick_test() {
let src = r#"
function test() {
return srcPipe.pipe(generator.stream).pipe(compile()).pipe(gulp.dest(out));
}"#;
const b4 = (
<div>
Text <a data-very-long-prop-breakline-rome-playground data-other>
some link
</a>{" "}
| some other text,{" "}
</div>
);
"#;
let syntax = SourceType::jsx();
let tree = parse(src, FileId::zero(), syntax);
let options = JsFormatOptions::new(syntax);
Expand Down
110 changes: 93 additions & 17 deletions crates/rome_js_formatter/src/utils/jsx.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,7 @@ pub(crate) fn jsx_split_children<I>(
where
I: IntoIterator<Item = JsxAnyChild>,
{
let mut result = Vec::new();
let mut builder = JsxSplitChildrenBuilder::new();

for child in children.into_iter() {
match child {
Expand Down Expand Up @@ -253,15 +253,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 @@ -274,15 +274,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 @@ -295,23 +295,50 @@ where

JsxAnyChild::JsxExpressionChild(child) => {
if is_whitespace_jsx_expression(&child, comments) {
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())
}

/// The builder is used to:
/// 1. Remove [JsxChild::EmptyLine], [JsxChild::Newline], [JsxChild::Whitespace] if a next element is [JsxChild::Whitespace]
/// 2. Don't push a new element [JsxChild::EmptyLine], [JsxChild::Newline], [JsxChild::Whitespace] if previous one is [JsxChild::EmptyLine], [JsxChild::Newline], [JsxChild::Whitespace]
/// [Prettier applies]: https://github.com/prettier/prettier/blob/b0d9387b95cdd4e9d50f5999d3be53b0b5d03a97/src/language-js/print/jsx.js#L144-L180
#[derive(Debug)]
struct JsxSplitChildrenBuilder {
buffer: Vec<JsxChild>,
}

impl JsxSplitChildrenBuilder {
fn new() -> Self {
JsxSplitChildrenBuilder { buffer: vec![] }
}

fn entry(&mut self, child: JsxChild) {
match self.buffer.last_mut() {
Some(last @ (JsxChild::EmptyLine | JsxChild::Newline | JsxChild::Whitespace)) => {
if matches!(child, JsxChild::Whitespace) {
*last = child;
} else if matches!(child, JsxChild::NonText(_) | JsxChild::Word(_)) {
self.buffer.push(child);
}
}
_ => self.buffer.push(child),
}
}

Ok(result)
fn finish(self) -> Vec<JsxChild> {
self.buffer
}
}

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

impl JsxWord {
pub fn is_ascii_punctuation(&self) -> bool {
self.text.chars().count() == 1
&& self
.text
.chars()
.next()
.map_or(false, |char| char.is_ascii_punctuation())
}
}

impl Format<JsFormatContext> for JsxWord {
fn fmt(&self, f: &mut Formatter<JsFormatContext>) -> FormatResult<()> {
f.write_element(FormatElement::Text(Text::SyntaxTokenTextSlice {
Expand Down Expand Up @@ -645,6 +683,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, &Comments::default()).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, &Comments::default()).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
Loading

0 comments on commit 5831a07

Please sign in to comment.