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

denbezrukov · Sep 28, 2022 · d053d46 · d053d46
1 parent dd74470
commit d053d46
Show file tree

Hide file tree

Showing 6 changed files with 129 additions and 318 deletions.
diff --git a/crates/rome_js_formatter/src/jsx/lists/child_list.rs b/crates/rome_js_formatter/src/jsx/lists/child_list.rs
@@ -116,7 +116,7 @@ impl FormatJsxChildList {
                     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);
                     }
@@ -151,7 +151,7 @@ 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);
                     }
@@ -205,20 +205,32 @@ impl FormatJsxChildList {
 
                     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 {
+                            multiline.write_with_empty_separator(&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);
+                            flat.write(&format_args![memoized, format_separator], f);
+                        } else {
+                            multiline.write_with_empty_separator(&memoized, f);
+                            flat.write(&format_args![memoized], f);
+                        }
                     }
                 }
             }
@@ -481,19 +493,30 @@ impl MultilineBuilder {
     }
 
     /// Formats an element that does not require a 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_with_empty_separator(
         &mut self,
         content: &dyn Format<JsFormatContext>,
         f: &mut JsFormatter,
     ) {
-        self.write(content, &format_with(|_| Ok(())), f)
+        self.write(content, None, f);
     }
 
-    fn write(
+    fn write_with_separator(
         &mut self,
         content: &dyn Format<JsFormatContext>,
         separator: &dyn Format<JsFormatContext>,
         f: &mut JsFormatter,
+    ) {
+        self.write(content, Some(separator), f);
+    }
+
+    fn write(
+        &mut self,
+        content: &dyn Format<JsFormatContext>,
+        separator: Option<&dyn Format<JsFormatContext>>,
+        f: &mut JsFormatter,
     ) {
         let result = std::mem::replace(&mut self.result, Ok(Vec::new()));
 
@@ -507,12 +530,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()

diff --git a/crates/rome_js_formatter/src/lib.rs b/crates/rome_js_formatter/src/lib.rs
@@ -861,8 +861,14 @@ function() {
     // use this test check if your snippet prints as you wish, without using a snapshot
     fn quick_test() {
         let src = r#"
-const a = <><b/>a{' '}{" "}{" "}{" "}c{" "}{" "}{" "}{" "}{" "}{" "}</>;
-
+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();

diff --git a/crates/rome_js_formatter/src/utils/jsx.rs b/crates/rome_js_formatter/src/utils/jsx.rs
@@ -309,8 +309,9 @@ where
     Ok(builder.finish())
 }
 
-/// The builder removes [JsxChild::EmptyLine], [JsxChild::Newline], [JsxChild::Whitespace]
-/// if a next element is [JsxChild::Whitespace]
+/// 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 {
@@ -324,10 +325,12 @@ impl JsxSplitChildrenBuilder {
 
     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
+            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),
         }

diff --git a/crates/rome_js_formatter/tests/specs/jsx/element.jsx.snap b/crates/rome_js_formatter/tests/specs/jsx/element.jsx.snap
@@ -358,10 +358,7 @@ c2 = (
 // this group should fit one line jsx whitespaces are hidden
 b = (
 	<div>
-		<a></a>
-
-		<a></a>
-		1
+		<a></a> <a></a> 1
 	</div>
 );
 
@@ -374,17 +371,14 @@ b1 = (
 12312
 			`}
 		</a>{" "}
-
-		<a></a>
-		1
+		<a></a> 1
 	</div>
 );
 
 // this group fit one line and hide jsx whitespace
 b2 = (
 	<>
-		<a>123</a>
-		1
+		<a>123</a> 1
 	</>
 );
 
@@ -401,7 +395,8 @@ b3 = (
 
 const b4 = (
 	<div>
-		Text <a data-very-long-prop-breakline-rome-playground data-other>
+		Text{" "}
+		<a data-very-long-prop-breakline-rome-playground data-other>
 			some link
 		</a>{" "}
 		| some other text,{" "}
@@ -617,9 +612,9 @@ const breadcrumbItems = [
     2: <div tooltip="A very long tooltip text that would otherwise make the attribute break onto the same line but it is not because of the single string layout"></div>;
     7: 		tooltip="A very long tooltip text that would otherwise make the attribute break
    14: 	<ASuperLongComponentNameThatWouldBreakButDoesntSinceTheComponent></ASuperLongComponentNameThatWouldBreakButDoesntSinceTheComponent>
-  179: 							"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
-  200: 							"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
-  213: 							"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
-  238: 				<pre className="h-screen overflow-y-scroll whitespace-pre-wrap text-red-500 text-xs">
-  287: 		Uncle Boonmee Who Can Recall His Past Lives dir. Apichatpong Weerasethakul{" "}
+  174: 							"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
+  195: 							"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
+  208: 							"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
+  233: 				<pre className="h-screen overflow-y-scroll whitespace-pre-wrap text-red-500 text-xs">
+  282: 		Uncle Boonmee Who Can Recall His Past Lives dir. Apichatpong Weerasethakul{" "}
 
diff --git a/crates/rome_js_formatter/tests/specs/prettier/jsx/significant-space/test.js.snap b/crates/rome_js_formatter/tests/specs/prettier/jsx/significant-space/test.js.snap
@@ -89,39 +89,18 @@ not_broken_begin =
 ```diff
 --- Prettier
 +++ Rome
-@@ -12,8 +12,7 @@
+@@ -12,8 +12,9 @@
 
  before_break1 = (
    <span>
 -    <span barbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbar />{" "}
 -    foo
-+    <span barbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbar /> foo
-   </span>
- );
-
-@@ -28,8 +27,9 @@
-
- after_break = (
-   <span>
--    foo{" "}
--    <span barbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbar />
-+    foo <span
++    <span
 +      barbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbar
-+    />
++    /> foo
    </span>
  );
 
-@@ -111,7 +111,8 @@
-
- not_broken_begin = (
-   <div>
--    <br /> long text long text long text long text long text long text long text
--    long text<link>url</link> long text long text
-+    <br /> long text long text long text long text long text long text long text long text<link>
-+      url
-+    </link> long text long text
-   </div>
- );
 ```
 
 # Output
@@ -141,7 +120,9 @@ before = (
 
 before_break1 = (
   <span>
-    <span barbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbar /> foo
+    <span
+      barbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbar
+    /> foo
   </span>
 );
 
@@ -156,9 +137,8 @@ before_break2 = (
 
 after_break = (
   <span>
-    foo <span
-      barbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbar
-    />
+    foo{" "}
+    <span barbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbar />
   </span>
 );
 
@@ -240,18 +220,15 @@ not_broken_end = (
 
 not_broken_begin = (
   <div>
-    <br /> long text long text long text long text long text long text long text long text<link>
-      url
-    </link> long text long text
+    <br /> long text long text long text long text long text long text long text
+    long text<link>url</link> long text long text
   </div>
 );
 ```
 
 
 # Lines exceeding max width of 80 characters
 ```
-   15:     <span barbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbarbar /> foo
-   95:     <Icon icon="aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" />{" "}
-  114:     <br /> long text long text long text long text long text long text long text long text<link>
+   96:     <Icon icon="aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa" />{" "}
 ```