From aad06a6f4b557203958b7af6427b1f9a5db926ed Mon Sep 17 00:00:00 2001 From: Denis Bezrukov <6227442+denbezrukov@users.noreply.github.com> Date: Mon, 19 Sep 2022 10:59:00 +0300 Subject: [PATCH] fix(rome_js_formatter): in JSX, some spaces are removed #3211 --- .../src/jsx/lists/child_list.rs | 12 +- crates/rome_js_formatter/src/lib.rs | 3 +- crates/rome_js_formatter/src/utils/jsx.rs | 118 ++++++++++++-- .../tests/specs/jsx/element.jsx | 69 ++++++++ .../tests/specs/jsx/element.jsx.snap | 154 +++++++++++++++++- 5 files changed, 329 insertions(+), 27 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 index ad9d75ad099d..e06c19f0662f 100644 --- a/crates/rome_js_formatter/src/jsx/lists/child_list.rs +++ b/crates/rome_js_formatter/src/jsx/lists/child_list.rs @@ -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 { @@ -142,7 +143,7 @@ impl FormatRule 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 // 
adefg
@@ -152,7 +153,9 @@ impl FormatRule for FormatJsxChildList {
                             // 
                             // 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)
@@ -160,8 +163,11 @@ impl FormatRule for FormatJsxChildList {
                         }
 
                         // 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,
                     };
diff --git a/crates/rome_js_formatter/src/lib.rs b/crates/rome_js_formatter/src/lib.rs
index 2de1638c79e4..7900b5f33741 100644
--- a/crates/rome_js_formatter/src/lib.rs
+++ b/crates/rome_js_formatter/src/lib.rs
@@ -751,7 +751,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 = <>a{' '}{" "}{" "}{" "}c{" "}{" "}{" "}{" "}{" "}{" "};
+
 
 "#;
         let syntax = SourceType::tsx();
diff --git a/crates/rome_js_formatter/src/utils/jsx.rs b/crates/rome_js_formatter/src/utils/jsx.rs
index 39e849c0c646..ec3a354261ff 100644
--- a/crates/rome_js_formatter/src/utils/jsx.rs
+++ b/crates/rome_js_formatter/src/utils/jsx.rs
@@ -122,7 +122,7 @@ impl Format 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())
             ]
         ]
@@ -173,7 +173,7 @@ pub(crate) fn jsx_split_children(children: I) -> SyntaxResult>
 where
     I: IntoIterator,
 {
-    let mut result = Vec::new();
+    let mut builder = JsxSplitChildrenBuilder::default();
 
     for child in children.into_iter() {
         match child {
@@ -203,15 +203,15 @@ where
                                     // 
                                     // ```
                                     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!(),
@@ -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())),
@@ -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,
+    buffer: Vec,
+}
+
+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 {
+        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)]
@@ -328,6 +366,12 @@ pub(crate) struct JsxWord {
     source_position: TextSize,
 }
 
+impl JsxWord {
+    pub fn len(&self) -> TextSize {
+        self.text.len()
+    }
+}
+
 impl Format for JsxWord {
     fn fmt(&self, f: &mut Formatter) -> FormatResult<()> {
         f.write_element(FormatElement::Text(Text::SyntaxTokenTextSlice {
@@ -594,6 +638,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) => {
diff --git a/crates/rome_js_formatter/tests/specs/jsx/element.jsx b/crates/rome_js_formatter/tests/specs/jsx/element.jsx
index 77f6f14a310a..b22afc059328 100644
--- a/crates/rome_js_formatter/tests/specs/jsx/element.jsx
+++ b/crates/rome_js_formatter/tests/specs/jsx/element.jsx
@@ -41,6 +41,75 @@ c = 
a{' '}{' '}{' '}{' '}{' '}{' '}{' '}{' '}b{' '}{' '}{' '}{' '}{' '}{' '
 
 c2 = 
a{' '}{' '}{' '}{' '}{' '}{' '}{' '}{' '}
content{' '}{' '}{' '}{' '}{' '}{' '}
;
 
+// this group should fit one line jsx whitespaces are hidden
+b =
+	

+		
+
+		{' '}
+
+		
+
+		{' '}
+		1
+	
;
+
+// this group should break first tag and show only first jsx whitespace
+b1 =
+	

+		
+			{`
+12312
+12312
+			`}
+		
+
+		{' '}
+
+		
+
+		{' '}
+		1
+	
;
+
+// this group fit one line and hide jsx whitespace
+b2 =
+	<>
+		123
+		
+
+		{' '}
+		1
+	;
+
+// this group break group and show jsx whitespace
+b3 =
+	<>
+		{`
+123`}
+		
+
+		{' '}
+		1
+	;
+
+// one length text should insert soft line break
+b4 = <>
a;
+// longer than one length text should insert hard line break
+b5 = <>
ab;
+
+// one length text should insert soft line break show last jsx whitespace
+b6 = <>a{' '}bb{" "}cc{" "}dd{" "}ee{" "}{" "}{" "}{" "}{" "}{" "};
+// longer than one length text should insert hard line break
+b7 = <>longer{' '}bb{" "}cc{" "}dd{" "}ee{" "}{" "}{" "}{" "}{" "}{" "};
+
+const b8 = 

+	Text 
+	some link
+{' '}
+	| some other text,{' '}
+
;
+
 ;
 
 const Essay = () => 
The films of Wong Kar-Wai exemplify the synthesis of French New Wave cinema—specifically the unrelenting
diff --git a/crates/rome_js_formatter/tests/specs/jsx/element.jsx.snap b/crates/rome_js_formatter/tests/specs/jsx/element.jsx.snap
index 7eac0eacc568..d8ca63b7005e 100644
--- a/crates/rome_js_formatter/tests/specs/jsx/element.jsx.snap
+++ b/crates/rome_js_formatter/tests/specs/jsx/element.jsx.snap
@@ -46,6 +46,75 @@ c = 
a{' '}{' '}{' '}{' '}{' '}{' '}{' '}{' '}b{' '}{' '}{' '}{' '}{' '}{' '
 
 c2 = 
a{' '}{' '}{' '}{' '}{' '}{' '}{' '}{' '}
content{' '}{' '}{' '}{' '}{' '}{' '}
;
 
+// this group should fit one line jsx whitespaces are hidden
+b =
+	

+		
+
+		{' '}
+
+		
+
+		{' '}
+		1
+	
;
+
+// this group should break first tag and show only first jsx whitespace
+b1 =
+	

+		
+			{`
+12312
+12312
+			`}
+		
+
+		{' '}
+
+		
+
+		{' '}
+		1
+	
;
+
+// this group fit one line and hide jsx whitespace
+b2 =
+	<>
+		123
+		
+
+		{' '}
+		1
+	;
+
+// this group break group and show jsx whitespace
+b3 =
+	<>
+		{`
+123`}
+		
+
+		{' '}
+		1
+	;
+
+// one length text should insert soft line break
+b4 = <>
a;
+// longer than one length text should insert hard line break
+b5 = <>
ab;
+
+// one length text should insert soft line break show last jsx whitespace
+b6 = <>a{' '}bb{" "}cc{" "}dd{" "}ee{" "}{" "}{" "}{" "}{" "}{" "};
+// longer than one length text should insert hard line break
+b7 = <>longer{' '}bb{" "}cc{" "}dd{" "}ee{" "}{" "}{" "}{" "}{" "}{" "};
+
+const b8 = 

+	Text 
+	some link
+{' '}
+	| some other text,{' '}
+
;
+
 ;
 
 const Essay = () => 
The films of Wong Kar-Wai exemplify the synthesis of French New Wave cinema—specifically the unrelenting
@@ -296,6 +365,81 @@ c2 = (
 	

 );
 
+// this group should fit one line jsx whitespaces are hidden
+b = (
+	

+		  1
+	

+);
+
+// this group should break first tag and show only first jsx whitespace
+b1 = (
+	
+);
+
+// this group fit one line and hide jsx whitespace
+b2 = (
+	<>
+		123 1
+	
+);
+
+// this group break group and show jsx whitespace
+b3 = (
+	<>
+		
+			{`
+123`}
+		{" "}
+		1
+	
+);
+
+// one length text should insert soft line break
+b4 = (
+	<>
+		
a
+	
+);
+// longer than one length text should insert hard line break
+b5 = (
+	<>
+		
+		ab
+	
+);
+
+// one length text should insert soft line break show last jsx whitespace
+b6 = (
+	<>
+		a bb cc dd ee{" "}
+	
+);
+// longer than one length text should insert hard line break
+b7 = (
+	<>
+		
+		longer bb cc dd ee{" "}
+	
+);
+
+const b8 = (
+	

+		Text 
+			some link
+		{" "}
+		| some other text,{" "}
+	

+);
+
 

 	

 	 jumps over the lazy dog 
@@ -504,9 +648,9 @@ const breadcrumbItems = [
     2: 
;
     7: 		tooltip="A very long tooltip text that would otherwise make the attribute break
    14: 	
-  126: 							"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
-  147: 							"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
-  160: 							"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
-  185: 				
-  234: 		Uncle Boonmee Who Can Recall His Past Lives dir. Apichatpong Weerasethakul{" "}
+  201: 							"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
+  222: 							"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
+  235: 							"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
+  260: 				
+  309: 		Uncle Boonmee Who Can Recall His Past Lives dir. Apichatpong Weerasethakul{" "}