From 24b13bba9e24dfa1b0e100159ad7d1b60e36ecf1 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 | 87 +++-- crates/rome_js_formatter/src/lib.rs | 13 +- crates/rome_js_formatter/src/utils/jsx.rs | 110 +++++- .../tests/specs/jsx/element.jsx | 64 ++++ .../tests/specs/jsx/element.jsx.snap | 129 ++++++- .../jsx/significant-space/test.js.snap | 94 +---- .../jsx/stateless-arrow-fn/test.js.snap | 300 ---------------- .../specs/prettier/jsx/text-wrap/test.js.snap | 334 +++--------------- 8 files changed, 425 insertions(+), 706 deletions(-) delete mode 100644 crates/rome_js_formatter/tests/specs/prettier/jsx/stateless-arrow-fn/test.js.snap 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 d36811fbdd2..ff42a88eeeb 100644 --- a/crates/rome_js_formatter/src/jsx/lists/child_list.rs +++ b/crates/rome_js_formatter/src/jsx/lists/child_list.rs @@ -108,7 +108,11 @@ impl FormatJsxChildList { ), }), - _ => None, + Some(JsxChild::Newline | JsxChild::Whitespace | JsxChild::EmptyLine) => { + None + } + + None => None, }; child_breaks = separator.map_or(false, |separator| separator.will_break()); @@ -116,9 +120,10 @@ 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); + // 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); } } @@ -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 // @@ -151,9 +156,9 @@ impl FormatJsxChildList { // // ``` 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); } } @@ -161,20 +166,20 @@ impl FormatJsxChildList { 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 //
adefg @@ -184,7 +189,9 @@ impl FormatJsxChildList { // // adefg // ``` - if matches!(non_text, JsxAnyChild::JsxSelfClosingElement(_)) { + if matches!(non_text, JsxAnyChild::JsxSelfClosingElement(_)) + && !word.is_ascii_punctuation() + { Some(LineMode::Hard) } else { Some(LineMode::Soft) @@ -192,28 +199,44 @@ impl 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, }; 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); + } } } } @@ -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". + fn write_content(&mut self, content: &dyn Format- 232: Uncle Boonmee Who Can Recall His Past Lives dir. Apichatpong Weerasethakul{" "} + 181: "ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace", + 202: "ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace", + 215: "ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace", + 240:+ 289: 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 index 054555963c5..1af27e8e61c 100644 --- 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,74 +89,18 @@ not_broken_begin = ```diff --- Prettier +++ Rome -@@ -12,7 +12,7 @@ +@@ -12,8 +12,9 @@ before_break1 = ( - {" "} -+ - foo - - ); -@@ -21,15 +21,16 @@ - - {" "} -+ /> - foo - - ); - - after_break = ( - -- foo{" "} -- -+ foo - - ); - -@@ -79,7 +80,8 @@ - - regression_not_transformed_2 = ( - --{" "} -+ -+ {" "} - - ); - -@@ -92,13 +94,15 @@ - - similar_2 = ( - -- {" "} -+ -+ {" "} - - ); - - similar_3 = ( - -- -+ -+ ++ /> foo ); -@@ -111,7 +115,8 @@ - - not_broken_begin = ( - --- ); ``` # Output @@ -176,8 +120,9 @@ before = ( before_break1 = ( - - foo + foo ); @@ -185,16 +130,15 @@ before_break2 = ( + />{" "} foo ); after_break = ( - foo + foo{" "} + ); @@ -244,8 +188,7 @@ regression_not_transformed_1 = ( regression_not_transformed_2 = ( -
long text long text long text long text long text long text long text -- long texturl long text long text -+
-+ long text long text long text long text long text long text long text long -+ texturl long text long text -- {" "} + {" "} ); @@ -258,15 +201,13 @@ similar_1 = ( similar_2 = ( - - {" "} + {" "} ); similar_3 = ( - - + ); @@ -279,12 +220,15 @@ not_broken_end = ( not_broken_begin = ( -); ``` +# Lines exceeding max width of 80 characters +``` + 96:
- long text long text long text long text long text long text long text long - texturl long text long text +
long text long text long text long text long text long text long text + long texturl long text long text{" "} +``` diff --git a/crates/rome_js_formatter/tests/specs/prettier/jsx/stateless-arrow-fn/test.js.snap b/crates/rome_js_formatter/tests/specs/prettier/jsx/stateless-arrow-fn/test.js.snap deleted file mode 100644 index bd71e968e67..00000000000 --- a/crates/rome_js_formatter/tests/specs/prettier/jsx/stateless-arrow-fn/test.js.snap +++ /dev/null @@ -1,300 +0,0 @@ ---- -source: crates/rome_js_formatter/tests/prettier_tests.rs -info: - test_file: jsx/stateless-arrow-fn/test.js ---- - -# Input - -```js -const render1 = ({ styles }) => ( - - Keep the wrapping parens. Put each key on its own line. --); - -const render2 = ({ styles }) =>- Create wrapping parens. -; - -const render3 = ({ styles }) =>Create wrapping parens.; - -const render4 = ({ styles }) =>Create wrapping parens and indent all the things.; - -const render5 = ({ styles }) =>Keep it on one line.; - -const render6 = ({ styles }) => ( ---) - -const render7 = () => -ddd d dd d d dddd dddd hello-ddd d dd d d dddd dddd hello---ddd d dd d d dddd dddd hellohello- Dont break each elem onto its own line. - -- -const render7A = () => ( -- --) - -const render7B = () => ( -- Dont break plz - Dont break plz - Dont break plz --) - -const render8 = (props) =>{props.text}-const render9 = (props) =>{props.looooooooooooooooooooooooooooooong_text}-const render10 = (props) =>{props.even_looooooooooooooooooooooooooooooooooooooooooonger_contents}- -const notJSX = (aaaaaaaaaaaaaaaaa, bbbbbbbbbbb) => this.someLongCallWithParams(aaaaaa, bbbbbbb).anotherLongCallWithParams(cccccccccccc, dddddddddddddddddddddd) - -React.render( -- , document.querySelector('#react-root') -) - - -const renderTernary = (props) => - - {props.showTheThing ? - -``` - - -# Prettier differences - -```diff ---- Prettier -+++ Rome -@@ -55,7 +55,7 @@ - attr4 - > - ddd d dd d d dddd dddd hello --Hello world - : "hello " + "howdy! "} - {props.showTheThing ? -Hello world - : - null - } - {props.showTheThing ? null : -Hello world - } - {props.showTheOtherThing ?I am here: } - {props.showTheOtherThing ?I am here!!: null} -