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

Commit

Permalink
fix(rome_js_formatter): jsx punctuation (#3842)
Browse files Browse the repository at this point in the history 
Fix #3531 (comment)
  • Loading branch information
@denbezrukov
denbezrukov authored Nov 28, 2022
1 parent 67a8881 commit 76689b2
Show file tree
Hide file tree
Showing 6 changed files with 385 additions and 121 deletions.
82 changes: 59 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
@@ -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};
Expand Down Expand Up @@ -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())?;
Expand All @@ -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() {
Expand All @@ -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) => {
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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>;
Expand All @@ -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 {
Expand All @@ -432,7 +471,7 @@ impl WordSeparator {
matches!(
self,
WordSeparator::EndOfText {
is_next_self_closing: true
is_soft_line_break: false,
}
)
}
Expand All @@ -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>
// ```
Expand All @@ -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)
}
}
}
Expand Down
109 changes: 101 additions & 8 deletions crates/rome_js_formatter/src/utils/jsx.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)));
}
}
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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}</>"),
Expand Down
50 changes: 50 additions & 0 deletions crates/rome_js_formatter/tests/specs/jsx/new-lines.jsx
View file
Original file line number Diff line number Diff line change
@@ -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>
);
Loading

0 comments on commit 76689b2

Please sign in to comment.