Skip to content

Commit 5831a07

Browse files
denbezrukovdenbezrukov
committed
fix(rome_js_formatter): in JSX, some spaces are removed rome#3211 
1 parent c876290 commit 5831a07

File tree

8 files changed

+407
-912
lines changed

8 files changed

+407
-912
lines changed

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

+64-23
Original file line numberDiff line numberDiff line change
@@ -108,17 +108,22 @@ impl FormatJsxChildList {
108108
),
109109
}),
110110

111-
_ => None,
111+
Some(JsxChild::Newline | JsxChild::Whitespace | JsxChild::EmptyLine) => {
112+
None
113+
}
114+
115+
None => None,
112116
};
113117

114118
child_breaks = separator.map_or(false, |separator| separator.will_break());
115119

116120
flat.write(&format_args![word, separator], f);
117121

118122
if let Some(separator) = separator {
119-
multiline.write(word, &separator, f);
123+
multiline.write_with_separator(word, &separator, f);
120124
} else {
121-
multiline.write_with_empty_separator(word, f);
125+
// it's safe to write without a separator because None means that next element is a separator or end of the iterator
126+
multiline.write_content(word, f);
122127
}
123128
}
124129

@@ -139,7 +144,7 @@ impl FormatJsxChildList {
139144
let is_trailing_or_only_whitespace = children_iter.peek().is_none();
140145

141146
if is_trailing_or_only_whitespace || is_after_line_break {
142-
multiline.write_with_empty_separator(&JsxRawSpace, f);
147+
multiline.write_separator(&JsxRawSpace, f);
143148
}
144149
// Leading whitespace. Only possible if used together with a expression child
145150
//
@@ -151,30 +156,30 @@ impl FormatJsxChildList {
151156
// </div>
152157
// ```
153158
else if last.is_none() {
154-
multiline.write(&JsxRawSpace, &hard_line_break(), f);
159+
multiline.write_with_separator(&JsxRawSpace, &hard_line_break(), f);
155160
} else {
156-
multiline.write_with_empty_separator(&JsxSpace, f);
161+
multiline.write_separator(&JsxSpace, f);
157162
}
158163
}
159164

160165
// A new line between some JSX text and an element
161166
JsxChild::Newline => {
162167
child_breaks = true;
163168

164-
multiline.write_with_empty_separator(&hard_line_break(), f);
169+
multiline.write_separator(&hard_line_break(), f);
165170
}
166171

167172
// An empty line between some JSX text and an element
168173
JsxChild::EmptyLine => {
169174
child_breaks = true;
170175

171-
multiline.write_with_empty_separator(&empty_line(), f);
176+
multiline.write_separator(&empty_line(), f);
172177
}
173178

174179
// Any child that isn't text
175180
JsxChild::NonText(non_text) => {
176181
let line_mode = match children_iter.peek() {
177-
Some(JsxChild::Newline | JsxChild::Word(_) | JsxChild::Whitespace) => {
182+
Some(JsxChild::Word(word)) => {
178183
// Break if the current or next element is a self closing element
179184
// ```javascript
180185
// <pre className="h-screen overflow-y-scroll" />adefg
@@ -184,36 +189,54 @@ impl FormatJsxChildList {
184189
// <pre className="h-screen overflow-y-scroll" />
185190
// adefg
186191
// ```
187-
if matches!(non_text, JsxAnyChild::JsxSelfClosingElement(_)) {
192+
if matches!(non_text, JsxAnyChild::JsxSelfClosingElement(_))
193+
&& !word.is_ascii_punctuation()
194+
{
188195
Some(LineMode::Hard)
189196
} else {
190197
Some(LineMode::Soft)
191198
}
192199
}
193200

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

204+
Some(JsxChild::Newline | JsxChild::Whitespace | JsxChild::EmptyLine) => {
205+
None
206+
}
197207
// Don't insert trailing line breaks
198208
None => None,
199209
};
200210

201211
child_breaks = line_mode.map_or(false, |mode| mode.is_hard());
202212

203-
let format_separator = format_with(|f| match line_mode {
204-
Some(mode) => f.write_element(FormatElement::Line(mode)),
205-
None => Ok(()),
213+
let format_separator = line_mode.map(|mode| {
214+
format_with(move |f| f.write_element(FormatElement::Line(mode)))
206215
});
207216

208217
if force_multiline {
209-
multiline.write(&non_text.format(), &format_separator, f);
218+
if let Some(format_separator) = format_separator {
219+
multiline.write_with_separator(
220+
&non_text.format(),
221+
&format_separator,
222+
f,
223+
);
224+
} else {
225+
// it's safe to write without a separator because None means that next element is a separator or end of the iterator
226+
multiline.write_content(&non_text.format(), f);
227+
}
210228
} else {
211229
let mut memoized = non_text.format().memoized();
212230

213231
force_multiline = memoized.inspect(f)?.will_break();
214-
215232
flat.write(&format_args![memoized, format_separator], f);
216-
multiline.write(&memoized, &format_separator, f);
233+
234+
if let Some(format_separator) = format_separator {
235+
multiline.write_with_separator(&memoized, &format_separator, f);
236+
} else {
237+
// it's safe to write without a separator because None means that next element is a separator or end of the iterator
238+
multiline.write_content(&memoized, f);
239+
}
217240
}
218241
}
219242
}
@@ -476,18 +499,30 @@ impl MultilineBuilder {
476499
}
477500

478501
/// Formats an element that does not require a separator
479-
fn write_with_empty_separator(
502+
/// It is safe to omit the separator because at the call side we must guarantee that we have reached the end of the iterator
503+
/// or the next element is a space/newline that should be written into the separator "slot".
504+
fn write_content(&mut self, content: &dyn Format<JsFormatContext>, f: &mut JsFormatter) {
505+
self.write(content, None, f);
506+
}
507+
508+
/// Formatting a separator does not require any element in the separator slot
509+
fn write_separator(&mut self, separator: &dyn Format<JsFormatContext>, f: &mut JsFormatter) {
510+
self.write(separator, None, f);
511+
}
512+
513+
fn write_with_separator(
480514
&mut self,
481515
content: &dyn Format<JsFormatContext>,
516+
separator: &dyn Format<JsFormatContext>,
482517
f: &mut JsFormatter,
483518
) {
484-
self.write(content, &format_with(|_| Ok(())), f)
519+
self.write(content, Some(separator), f);
485520
}
486521

487522
fn write(
488523
&mut self,
489524
content: &dyn Format<JsFormatContext>,
490-
separator: &dyn Format<JsFormatContext>,
525+
separator: Option<&dyn Format<JsFormatContext>>,
491526
f: &mut JsFormatter,
492527
) {
493528
let result = std::mem::replace(&mut self.result, Ok(Vec::new()));
@@ -502,12 +537,18 @@ impl MultilineBuilder {
502537
write!(buffer, [content])?;
503538
buffer.write_element(FormatElement::Tag(Tag::EndEntry))?;
504539

505-
buffer.write_element(FormatElement::Tag(Tag::StartEntry))?;
506-
write!(buffer, [separator])?;
507-
buffer.write_element(FormatElement::Tag(Tag::EndEntry))?;
540+
if let Some(separator) = separator {
541+
buffer.write_element(FormatElement::Tag(Tag::StartEntry))?;
542+
write!(buffer, [separator])?;
543+
buffer.write_element(FormatElement::Tag(Tag::EndEntry))?;
544+
}
508545
}
509546
MultilineLayout::NoFill => {
510547
write!(buffer, [content, separator])?;
548+
549+
if let Some(separator) = separator {
550+
write!(buffer, [separator])?;
551+
}
511552
}
512553
};
513554
buffer.into_vec()

‎crates/rome_js_formatter/src/lib.rs

+10-3
Original file line numberDiff line numberDiff line change
@@ -861,9 +861,16 @@ 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-
function test() {
865-
return srcPipe.pipe(generator.stream).pipe(compile()).pipe(gulp.dest(out));
866-
}"#;
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+
);
872+
873+
"#;
867874
let syntax = SourceType::jsx();
868875
let tree = parse(src, FileId::zero(), syntax);
869876
let options = JsFormatOptions::new(syntax);

‎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)