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

fix(rome_js_formatter): in JSX, some spaces are removed #3211 #3251

Merged
merged 1 commit into from
Oct 10, 2022
Merged

fix(rome_js_formatter): in JSX, some spaces are removed #3211 #3251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
87 changes: 64 additions & 23 deletions crates/rome_js_formatter/src/jsx/lists/child_list.rs
View file
Open in desktop
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) => {
MichaReiser marked this conversation as resolved.
Show resolved Hide resolved
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".
denbezrukov marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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
Open in desktop
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 {
MichaReiser marked this conversation as resolved.
Show resolved Hide resolved
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