fix(rome_js_formatter): jsx punctuation

crates/rome_js_formatter/src/jsx/lists/child_list.rs

@@ -1,7 +1,7 @@
 use crate::prelude::*;
 use crate::utils::jsx::{
     is_meaningful_jsx_text, is_whitespace_jsx_expression, jsx_split_children, JsxChild,
-    JsxRawSpace, JsxSpace,
+    JsxChildrenIterator, JsxRawSpace, JsxSpace,
 };
 use crate::JsFormatter;
 use rome_formatter::format_element::tag::{GroupMode, Tag};
@@ -72,7 +72,6 @@ impl FormatJsxChildList {
         let mut flat = FlatBuilder::new();
         let mut multiline = MultilineBuilder::new(multiline_layout);
 
-        let mut last: Option<JsxChild> = None;
         let mut force_multiline = layout.is_multiline();
 
         let mut children = jsx_split_children(list, f.context().comments())?;
@@ -82,7 +81,8 @@ impl FormatJsxChildList {
             children.pop();
         }
 
-        let mut children_iter = children.into_iter().peekable();
+        let mut last: Option<&JsxChild> = None;
+        let mut children_iter = JsxChildrenIterator::new(children.iter());
 
         // Trim leading new lines
         if let Some(JsxChild::Newline | JsxChild::EmptyLine) = children_iter.peek() {
@@ -102,11 +102,11 @@ impl FormatJsxChildList {
                         }
 
                         // Last word or last word before an element without any whitespace in between
-                        Some(JsxChild::NonText(child)) => Some(WordSeparator::EndOfText {
-                            is_next_self_closing: matches!(
-                                child,
+                        Some(JsxChild::NonText(next_child)) => Some(WordSeparator::EndOfText {
+                            is_soft_line_break: !matches!(
+                                next_child,
                                 JsxAnyChild::JsxSelfClosingElement(_)
-                            ),
+                            ) || word.is_ascii_punctuation(),
                         }),
 
                         Some(JsxChild::Newline | JsxChild::Whitespace | JsxChild::EmptyLine) => {
@@ -165,9 +165,50 @@ impl FormatJsxChildList {
 
                 // A new line between some JSX text and an element
                 JsxChild::Newline => {
-                    child_breaks = true;
+                    let is_soft_break = {
+                        // Here we handle the case when we have a newline between an ascii punctuation word and a jsx element
+                        // We need to use the previous and the next element
+                        // [JsxChild::Word, JsxChild::Newline, JsxChild::NonText]
+                        // ```
+                        // <div>
+                        //   <div>First</div>,
+                        //   <div>Second</div>
+                        // </div>
+                        // ```
+                        if let Some(JsxChild::Word(word)) = last {
+                            let is_next_element_self_closing = matches!(
+                                children_iter.peek(),
+                                Some(JsxChild::NonText(JsxAnyChild::JsxSelfClosingElement(_)))
+                            );
+                            !is_next_element_self_closing && word.is_ascii_punctuation()
+                        }
+                        // Here we handle the case when we have an ascii punctuation word between a new line and a jsx element
+                        // Here we need to look ahead two elements
+                        // [JsxChild::Newline, JsxChild::Word, JsxChild::NonText]
+                        // ```
+                        // <div>
+                        //   <div>First</div>
+                        //   ,<div>Second</div>
+                        // </div>
+                        // ```
+                        else if let Some(JsxChild::Word(next_word)) = children_iter.peek() {
+                            let is_next_next_element_self_closing = matches!(
+                                children_iter.peek_next(),
+                                Some(JsxChild::NonText(JsxAnyChild::JsxSelfClosingElement(_)))
+                            );
+
+                            !is_next_next_element_self_closing && next_word.is_ascii_punctuation()
+                        } else {
+                            false
+                        }
+                    };
 
-                    multiline.write_separator(&hard_line_break(), f);
+                    if is_soft_break {
+                        multiline.write_separator(&soft_line_break(), f);
+                    } else {
+                        child_breaks = true;
+                        multiline.write_separator(&hard_line_break(), f);
+                    }
                 }
 
                 // An empty line between some JSX text and an element
@@ -406,7 +447,8 @@ enum WordSeparator {
     /// <div>a{expression}</div> // last element before expression
     /// ```
     ///
-    /// Creates a soft line break EXCEPT if the next element is a self closing element, which results in a hard line break:
+    /// Creates a soft line break EXCEPT if the next element is a self closing element
+    /// or the previous word was an ascii punctuation, which results in a hard line break:
     ///
     /// ```javascript
     /// a = <div>ab<br/></div>;
@@ -420,10 +462,7 @@ enum WordSeparator {
     ///     </div>
     /// );
     /// ```
-    EndOfText {
-        /// `true` if the next element is a [JsxSelfClosingElement]
-        is_next_self_closing: bool,
-    },
+    EndOfText { is_soft_line_break: bool },
 }
 
 impl WordSeparator {
@@ -432,7 +471,7 @@ impl WordSeparator {
         matches!(
             self,
             WordSeparator::EndOfText {
-                is_next_self_closing: true
+                is_soft_line_break: false,
             }
         )
     }
@@ -442,9 +481,10 @@ impl Format<JsFormatContext> for WordSeparator {
     fn fmt(&self, f: &mut Formatter<JsFormatContext>) -> FormatResult<()> {
         match self {
             WordSeparator::BetweenWords => soft_line_break_or_space().fmt(f),
-            WordSeparator::EndOfText {
-                is_next_self_closing: self_closing,
-            } => {
+            WordSeparator::EndOfText { is_soft_line_break } => {
+                if *is_soft_line_break {
+                    soft_line_break().fmt(f)
+                }
                 // ```javascript
                 // <div>ab<br/></div>
                 // ```
@@ -456,12 +496,8 @@ impl Format<JsFormatContext> for WordSeparator {
                 //  <br />
                 // </div>
                 // ```
-                if *self_closing {
-                    hard_line_break().fmt(f)
-                }
-                // Try to fit everything else on a single line
                 else {
-                    soft_line_break().fmt(f)
+                    hard_line_break().fmt(f)
                 }
             }
         }

crates/rome_js_formatter/src/utils/jsx.rs

@@ -282,12 +282,12 @@ where
                         }
 
                         (relative_start, JsxTextChunk::Word(word)) => {
-                            builder.entry(JsxChild::Word(JsxWord {
-                                text: value_token
-                                    .token_text()
-                                    .slice(TextRange::at(relative_start, word.text_len())),
-                                source_position: value_token.text_range().start() + relative_start,
-                            }));
+                            let text = value_token
+                                .token_text()
+                                .slice(TextRange::at(relative_start, word.text_len()));
+                            let source_position = value_token.text_range().start() + relative_start;
+
+                            builder.entry(JsxChild::Word(JsxWord::new(text, source_position)));
                         }
                     }
                 }
@@ -406,7 +406,14 @@ pub(crate) struct JsxWord {
 }
 
 impl JsxWord {
-    pub fn is_ascii_punctuation(&self) -> bool {
+    fn new(text: SyntaxTokenText, source_position: TextSize) -> Self {
+        JsxWord {
+            text,
+            source_position,
+        }
+    }
+
+    pub(crate) fn is_ascii_punctuation(&self) -> bool {
         self.text.chars().count() == 1
             && self
                 .text
@@ -487,15 +494,101 @@ impl<'a> Iterator for JsxSplitChunksIterator<'a> {
 
 impl FusedIterator for JsxSplitChunksIterator<'_> {}
 
+/// An iterator adaptor that allows a lookahead of two tokens
+///
+/// # Examples
+/// ```
+/// use rome_js_formatter::utils::jsx::JsxChildrenIterator;
+///
+/// let buffer = vec![1, 2, 3, 4];
+///
+/// let mut iter = JsxChildrenIterator::new(buffer.iter());
+///
+/// assert_eq!(iter.peek(), Some(&&1));
+/// assert_eq!(iter.peek_next(), Some(&&2));
+/// assert_eq!(iter.next(), Some(&1));
+/// assert_eq!(iter.next(), Some(&2));
+/// ```
+#[derive(Clone, Debug)]
+pub struct JsxChildrenIterator<I: Iterator> {
+    iter: I,
+
+    peeked: Option<Option<I::Item>>,
+    peeked_next: Option<Option<I::Item>>,
+}
+
+impl<I: Iterator> JsxChildrenIterator<I> {
+    pub fn new(iter: I) -> Self {
+        Self {
+            iter,
+            peeked: None,
+            peeked_next: None,
+        }
+    }
+
+    pub fn peek(&mut self) -> Option<&I::Item> {
+        let iter = &mut self.iter;
+        self.peeked.get_or_insert_with(|| iter.next()).as_ref()
+    }
+
+    pub fn peek_next(&mut self) -> Option<&I::Item> {
+        let iter = &mut self.iter;
+        let peeked = &mut self.peeked;
+
+        self.peeked_next
+            .get_or_insert_with(|| {
+                peeked.get_or_insert_with(|| iter.next());
+                iter.next()
+            })
+            .as_ref()
+    }
+}
+
+impl<I: Iterator> Iterator for JsxChildrenIterator<I> {
+    type Item = I::Item;
+
+    fn next(&mut self) -> Option<Self::Item> {
+        match self.peeked.take() {
+            Some(peeked) => {
+                self.peeked = self.peeked_next.take();
+                peeked
+            }
+            None => self.iter.next(),
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
-    use crate::utils::jsx::{jsx_split_children, JsxChild, JsxSplitChunksIterator, JsxTextChunk};
+    use crate::utils::jsx::{
+        jsx_split_children, JsxChild, JsxChildrenIterator, JsxSplitChunksIterator, JsxTextChunk,
+    };
     use rome_diagnostics::location::FileId;
     use rome_formatter::comments::Comments;
     use rome_js_parser::parse;
     use rome_js_syntax::{JsxChildList, JsxText, SourceType};
     use rome_rowan::{AstNode, TextSize};
 
+    #[test]
+    fn jsx_children_iterator_test() {
+        let buffer = vec![1, 2, 3, 4];
+
+        let mut iter = JsxChildrenIterator::new(buffer.iter());
+
+        assert_eq!(iter.peek(), Some(&&1));
+        assert_eq!(iter.peek(), Some(&&1));
+        assert_eq!(iter.peek_next(), Some(&&2));
+        assert_eq!(iter.peek_next(), Some(&&2));
+
+        assert_eq!(iter.next(), Some(&1));
+        assert_eq!(iter.next(), Some(&2));
+
+        assert_eq!(iter.peek_next(), Some(&&4));
+        assert_eq!(iter.peek_next(), Some(&&4));
+        assert_eq!(iter.peek(), Some(&&3));
+        assert_eq!(iter.peek(), Some(&&3));
+    }
+
     fn assert_jsx_text_chunks(text: &str, expected_chunks: Vec<(TextSize, JsxTextChunk)>) {
         let parse = parse(
             &std::format!("<>{text}</>"),

crates/rome_js_formatter/tests/specs/jsx/new-lines.jsx

@@ -0,0 +1,50 @@
+
+
+x = (
+	<div>
+		<div>First</div>,<div>Second</div>
+	</div>
+);
+
+x = (
+	<div>
+		<div>First</div>,
+		<div>Second</div>
+	</div>
+);
+
+x = (
+	<div>
+		<div>First</div>
+		,<div>Second</div>
+	</div>
+);
+
+function Component() {
+	return (
+		<>
+			<span>text</span>.<br />
+		</>
+	);
+}
+
+let myDiv1 = ReactTestUtils.renderIntoDocument(
+	<div1>
+		<div key="theDog" className="dog" />,<di key="theBird" className="bird" />
+	</div1>
+);
+
+
+let myDiv2 = ReactTestUtils.renderIntoDocument(
+	<div1>
+		<div key="theDog" className="dog" />
+		,<di key="theBird" className="bird" />
+	</div1>
+);
+
+let myDiv3 = ReactTestUtils.renderIntoDocument(
+	<div1>
+		<div key="theDog" className="dog" />,
+		<di key="theBird" className="bird" />
+	</div1>
+);