Skip to content

Commit e21972b

Browse files
denbezrukovdenbezrukov
committed
fix(rome_js_formatter): in JSX, some spaces are removed rome#3211 
1 parent 73defc1 commit e21972b

File tree

8 files changed

+409
-700
lines changed

8 files changed

+409
-700
lines changed

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

+50-16
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ impl FormatJsxChildList {
116116
flat.write(&format_args![word, separator], f);
117117

118118
if let Some(separator) = separator {
119-
multiline.write(word, &separator, f);
119+
multiline.write_with_separator(word, &separator, f);
120120
} else {
121121
multiline.write_with_empty_separator(word, f);
122122
}
@@ -151,7 +151,7 @@ impl FormatJsxChildList {
151151
// </div>
152152
// ```
153153
else if last.is_none() {
154-
multiline.write(&JsxRawSpace, &hard_line_break(), f);
154+
multiline.write_with_separator(&JsxRawSpace, &hard_line_break(), f);
155155
} else {
156156
multiline.write_with_empty_separator(&JsxSpace, f);
157157
}
@@ -174,7 +174,7 @@ impl FormatJsxChildList {
174174
// Any child that isn't text
175175
JsxChild::NonText(non_text) => {
176176
let line_mode = match children_iter.peek() {
177-
Some(JsxChild::Newline | JsxChild::Word(_) | JsxChild::Whitespace) => {
177+
Some(JsxChild::Word(word)) => {
178178
// Break if the current or next element is a self closing element
179179
// ```javascript
180180
// <pre className="h-screen overflow-y-scroll" />adefg
@@ -184,36 +184,53 @@ impl FormatJsxChildList {
184184
// <pre className="h-screen overflow-y-scroll" />
185185
// adefg
186186
// ```
187-
if matches!(non_text, JsxAnyChild::JsxSelfClosingElement(_)) {
187+
if matches!(non_text, JsxAnyChild::JsxSelfClosingElement(_))
188+
&& !word.is_ascii_punctuation()
189+
{
188190
Some(LineMode::Hard)
189191
} else {
190192
Some(LineMode::Soft)
191193
}
192194
}
193195

194196
// Add a hard line break if what comes after the element is not a text or is all whitespace
195-
Some(_) => Some(LineMode::Hard),
197+
Some(JsxChild::NonText(_)) => Some(LineMode::Hard),
196198

199+
Some(JsxChild::Newline | JsxChild::Whitespace | JsxChild::EmptyLine) => {
200+
None
201+
}
197202
// Don't insert trailing line breaks
198203
None => None,
199204
};
200205

201206
child_breaks = line_mode.map_or(false, |mode| mode.is_hard());
202207

203-
let format_separator = format_with(|f| match line_mode {
204-
Some(mode) => f.write_element(FormatElement::Line(mode)),
205-
None => Ok(()),
208+
let format_separator = line_mode.map(|mode| {
209+
format_with(move |f| f.write_element(FormatElement::Line(mode)))
206210
});
207211

208212
if force_multiline {
209-
multiline.write(&non_text.format(), &format_separator, f);
213+
if let Some(format_separator) = format_separator {
214+
multiline.write_with_separator(
215+
&non_text.format(),
216+
&format_separator,
217+
f,
218+
);
219+
} else {
220+
multiline.write_with_empty_separator(&non_text.format(), f);
221+
}
210222
} else {
211223
let mut memoized = non_text.format().memoized();
212224

213225
force_multiline = memoized.inspect(f)?.will_break();
214226

215-
flat.write(&format_args![memoized, format_separator], f);
216-
multiline.write(&memoized, &format_separator, f);
227+
if let Some(format_separator) = format_separator {
228+
multiline.write_with_separator(&memoized, &format_separator, f);
229+
flat.write(&format_args![memoized, format_separator], f);
230+
} else {
231+
multiline.write_with_empty_separator(&memoized, f);
232+
flat.write(&format_args![memoized], f);
233+
}
217234
}
218235
}
219236
}
@@ -476,19 +493,30 @@ impl MultilineBuilder {
476493
}
477494

478495
/// Formats an element that does not require a separator
496+
/// It is safe to omit the separator because at the call side we must guarantee that we have reached the end of the iterator
497+
/// or the next element is a space/newline that should be written into the separator "slot".
479498
fn write_with_empty_separator(
480499
&mut self,
481500
content: &dyn Format<JsFormatContext>,
482501
f: &mut JsFormatter,
483502
) {
484-
self.write(content, &format_with(|_| Ok(())), f)
503+
self.write(content, None, f);
485504
}
486505

487-
fn write(
506+
fn write_with_separator(
488507
&mut self,
489508
content: &dyn Format<JsFormatContext>,
490509
separator: &dyn Format<JsFormatContext>,
491510
f: &mut JsFormatter,
511+
) {
512+
self.write(content, Some(separator), f);
513+
}
514+
515+
fn write(
516+
&mut self,
517+
content: &dyn Format<JsFormatContext>,
518+
separator: Option<&dyn Format<JsFormatContext>>,
519+
f: &mut JsFormatter,
492520
) {
493521
let result = std::mem::replace(&mut self.result, Ok(Vec::new()));
494522

@@ -502,12 +530,18 @@ impl MultilineBuilder {
502530
write!(buffer, [content])?;
503531
buffer.write_element(FormatElement::Tag(Tag::EndEntry))?;
504532

505-
buffer.write_element(FormatElement::Tag(Tag::StartEntry))?;
506-
write!(buffer, [separator])?;
507-
buffer.write_element(FormatElement::Tag(Tag::EndEntry))?;
533+
if let Some(separator) = separator {
534+
buffer.write_element(FormatElement::Tag(Tag::StartEntry))?;
535+
write!(buffer, [separator])?;
536+
buffer.write_element(FormatElement::Tag(Tag::EndEntry))?;
537+
}
508538
}
509539
MultilineLayout::NoFill => {
510540
write!(buffer, [content, separator])?;
541+
542+
if let Some(separator) = separator {
543+
write!(buffer, [separator])?;
544+
}
511545
}
512546
};
513547
buffer.into_vec()

‎crates/rome_js_formatter/src/lib.rs

+8-4
Original file line numberDiff line numberDiff line change
@@ -861,10 +861,14 @@ function() {
861861
// use this test check if your snippet prints as you wish, without using a snapshot
862862
fn quick_test() {
863863
let src = r#"
864-
new Test()
865-
.test()
866-
.test([, 0])
867-
.test();
864+
const b4 = (
865+
<div>
866+
Text <a data-very-long-prop-breakline-rome-playground data-other>
867+
some link
868+
</a>{" "}
869+
| some other text,{" "}
870+
</div>
871+
);
868872
869873
"#;
870874
let syntax = SourceType::jsx();

‎crates/rome_js_formatter/src/utils/jsx.rs

+93-17
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ pub(crate) fn jsx_split_children<I>(
223223
where
224224
I: IntoIterator<Item = JsxAnyChild>,
225225
{
226-
let mut result = Vec::new();
226+
let mut builder = JsxSplitChildrenBuilder::new();
227227

228228
for child in children.into_iter() {
229229
match child {
@@ -253,15 +253,15 @@ where
253253
// </div>
254254
// ```
255255
if newlines > 1 {
256-
result.push(JsxChild::EmptyLine);
256+
builder.entry(JsxChild::EmptyLine);
257257
}
258258

259259
continue;
260260
}
261261

262-
result.push(JsxChild::Newline)
263-
} else if !matches!(result.last(), Some(JsxChild::Whitespace)) {
264-
result.push(JsxChild::Whitespace)
262+
builder.entry(JsxChild::Newline)
263+
} else {
264+
builder.entry(JsxChild::Whitespace)
265265
}
266266
}
267267
_ => unreachable!(),
@@ -274,15 +274,15 @@ where
274274
// Only handle trailing whitespace. Words must always be joined by new lines
275275
if chunks.peek().is_none() {
276276
if whitespace.contains('\n') {
277-
result.push(JsxChild::Newline);
277+
builder.entry(JsxChild::Newline);
278278
} else {
279-
result.push(JsxChild::Whitespace)
279+
builder.entry(JsxChild::Whitespace)
280280
}
281281
}
282282
}
283283

284284
(relative_start, JsxTextChunk::Word(word)) => {
285-
result.push(JsxChild::Word(JsxWord {
285+
builder.entry(JsxChild::Word(JsxWord {
286286
text: value_token
287287
.token_text()
288288
.slice(TextRange::at(relative_start, word.text_len())),
@@ -295,23 +295,50 @@ where
295295

296296
JsxAnyChild::JsxExpressionChild(child) => {
297297
if is_whitespace_jsx_expression(&child, comments) {
298-
match result.last() {
299-
Some(JsxChild::Whitespace) => {
300-
// Ignore
301-
}
302-
_ => result.push(JsxChild::Whitespace),
303-
}
298+
builder.entry(JsxChild::Whitespace)
304299
} else {
305-
result.push(JsxChild::NonText(child.into()))
300+
builder.entry(JsxChild::NonText(child.into()))
306301
}
307302
}
308303
child => {
309-
result.push(JsxChild::NonText(child));
304+
builder.entry(JsxChild::NonText(child));
305+
}
306+
}
307+
}
308+
309+
Ok(builder.finish())
310+
}
311+
312+
/// The builder is used to:
313+
/// 1. Remove [JsxChild::EmptyLine], [JsxChild::Newline], [JsxChild::Whitespace] if a next element is [JsxChild::Whitespace]
314+
/// 2. Don't push a new element [JsxChild::EmptyLine], [JsxChild::Newline], [JsxChild::Whitespace] if previous one is [JsxChild::EmptyLine], [JsxChild::Newline], [JsxChild::Whitespace]
315+
/// [Prettier applies]: https://github.com/prettier/prettier/blob/b0d9387b95cdd4e9d50f5999d3be53b0b5d03a97/src/language-js/print/jsx.js#L144-L180
316+
#[derive(Debug)]
317+
struct JsxSplitChildrenBuilder {
318+
buffer: Vec<JsxChild>,
319+
}
320+
321+
impl JsxSplitChildrenBuilder {
322+
fn new() -> Self {
323+
JsxSplitChildrenBuilder { buffer: vec![] }
324+
}
325+
326+
fn entry(&mut self, child: JsxChild) {
327+
match self.buffer.last_mut() {
328+
Some(last @ (JsxChild::EmptyLine | JsxChild::Newline | JsxChild::Whitespace)) => {
329+
if matches!(child, JsxChild::Whitespace) {
330+
*last = child;
331+
} else if matches!(child, JsxChild::NonText(_) | JsxChild::Word(_)) {
332+
self.buffer.push(child);
333+
}
310334
}
335+
_ => self.buffer.push(child),
311336
}
312337
}
313338

314-
Ok(result)
339+
fn finish(self) -> Vec<JsxChild> {
340+
self.buffer
341+
}
315342
}
316343

317344
#[derive(Debug, Clone, Eq, PartialEq)]
@@ -378,6 +405,17 @@ pub(crate) struct JsxWord {
378405
source_position: TextSize,
379406
}
380407

408+
impl JsxWord {
409+
pub fn is_ascii_punctuation(&self) -> bool {
410+
self.text.chars().count() == 1
411+
&& self
412+
.text
413+
.chars()
414+
.next()
415+
.map_or(false, |char| char.is_ascii_punctuation())
416+
}
417+
}
418+
381419
impl Format<JsFormatContext> for JsxWord {
382420
fn fmt(&self, f: &mut Formatter<JsFormatContext>) -> FormatResult<()> {
383421
f.write_element(FormatElement::Text(Text::SyntaxTokenTextSlice {
@@ -645,6 +683,44 @@ mod tests {
645683
assert_eq!(children[3], JsxChild::Whitespace);
646684
}
647685

686+
#[test]
687+
fn split_children_remove_in_row_jsx_whitespaces() {
688+
let child_list = parse_jsx_children(r#"a{' '}{' '}{' '}c{" "}{' '}{" "}"#);
689+
690+
let children = jsx_split_children(&child_list, &Comments::default()).unwrap();
691+
692+
assert_eq!(
693+
4,
694+
children.len(),
695+
"Expected to contain four elements. Actual:\n{children:#?} "
696+
);
697+
assert_word(&children[0], "a");
698+
assert_eq!(children[1], JsxChild::Whitespace);
699+
assert_word(&children[2], "c");
700+
assert_eq!(children[3], JsxChild::Whitespace);
701+
}
702+
703+
#[test]
704+
fn split_children_remove_new_line_before_jsx_whitespaces() {
705+
let child_list = parse_jsx_children(
706+
r#"a
707+
{' '}c{" "}
708+
"#,
709+
);
710+
711+
let children = jsx_split_children(&child_list, &Comments::default()).unwrap();
712+
713+
assert_eq!(
714+
4,
715+
children.len(),
716+
"Expected to contain four elements. Actual:\n{children:#?} "
717+
);
718+
assert_word(&children[0], "a");
719+
assert_eq!(children[1], JsxChild::Whitespace);
720+
assert_word(&children[2], "c");
721+
assert_eq!(children[3], JsxChild::Whitespace);
722+
}
723+
648724
fn assert_word(child: &JsxChild, text: &str) {
649725
match child {
650726
JsxChild::Word(word) => {

0 commit comments

Comments
 (0)