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

feat(rome_js_formatter): Format JSX #3144

Merged
merged 8 commits into from
Sep 2, 2022
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions crates/rome_formatter/src/buffer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,10 +164,11 @@ pub struct VecBuffer<'a, Context> {

impl<'a, Context> VecBuffer<'a, Context> {
pub fn new(state: &'a mut FormatState<Context>) -> Self {
Self {
state,
elements: vec![],
}
Self::new_with_vec(state, Vec::new())
}

pub fn new_with_vec(state: &'a mut FormatState<Context>, elements: Vec<FormatElement>) -> Self {
Self { state, elements }
}

/// Creates a buffer with the specified capacity
Expand Down
26 changes: 17 additions & 9 deletions crates/rome_formatter/src/format_element.rs
Original file line number Diff line number Diff line change
Expand Up @@ -224,6 +224,12 @@ pub enum LineMode {
Empty,
}

impl LineMode {
pub const fn is_hard(&self) -> bool {
matches!(self, LineMode::Hard)
}
}

/// A token used to gather a list of elements; see [crate::Formatter::join_with].
#[derive(Clone, Default, Eq, PartialEq)]
pub struct List {
Expand Down Expand Up @@ -984,9 +990,15 @@ impl Format<IrFormatContext> for FormatElement {
[
dynamic_text(&std::format!("<interned {index}>"), TextSize::default()),
space(),
&interned.0.as_ref()
]
)
)?;

match interned.0.as_ref() {
element @ FormatElement::Text(_) | element @ FormatElement::Space => {
write!(f, [text("\""), element, text("\"")])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes an issue where an interned string token wasn't put in quotes which made it very confusing when reading Rome's format IR

}
element => element.fmt(f),
}
} else {
write!(
f,
Expand Down Expand Up @@ -1017,13 +1029,9 @@ impl<'a> Format<IrFormatContext> for &'a [FormatElement] {
matches!(element, FormatElement::Text(_) | FormatElement::Space);

if print_as_str {
write!(f, [text("\"")])?;
}

write!(f, [group(&element)])?;

if print_as_str {
write!(f, [text("\"")])?;
write!(f, [text("\""), &element, text("\"")])?;
} else {
write!(f, [group(&element)])?;
}

if index < len - 1 {
Expand Down
3 changes: 2 additions & 1 deletion crates/rome_formatter/src/printed_tokens.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ impl PrintedTokens {
descendant_offset if descendant_offset < *offset => {
panic!("token has not been seen by the formatter: {descendant:#?}.\
\nUse `format_replaced` if you want to replace a token from the formatted output.\
\nUse `format_removed` if you to remove a token from the formatted output")
\nUse `format_removed` if you to remove a token from the formatted output.\n\
MichaReiser marked this conversation as resolved.
Show resolved Hide resolved
parent: {:#?}", descendant.parent())
}
descendant_offset if descendant_offset > *offset => {
panic!("tracked offset {offset:?} doesn't match any token of {root:#?}. Have you passed a token from another tree?");
Expand Down
22 changes: 19 additions & 3 deletions crates/rome_formatter/src/printer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -474,12 +474,28 @@ impl<'a> Printer<'a> {
current_fits = next_fits;
}
}
(Some(_), _) => {
// Don't print a trailing separator
// Trailing separator
(Some(separator), _) => {
let print_mode = if current_fits
&& fits_on_line(
[separator],
args.with_print_mode(PrintMode::Flat),
&empty_rest,
self,
) {
PrintMode::Flat
} else {
PrintMode::Expanded
};

self.print_all(queue, &[separator], args.with_print_mode(print_mode));
ematipico marked this conversation as resolved.
Show resolved Hide resolved
}
(None, _) => {
(None, None) => {
break;
}
(None, Some(_)) => {
unreachable!()
MichaReiser marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
use crate::prelude::*;

use rome_formatter::write;
use crate::utils::JsAnyBinaryLikeExpression;
use rome_formatter::{format_args, write};
use rome_js_syntax::{
JsAnyExpression, JsxExpressionAttributeValue, JsxExpressionAttributeValueFields,
JsAnyExpression, JsSyntaxNode, JsxAnyTag, JsxChildList, JsxExpressionAttributeValue,
JsxExpressionAttributeValueFields,
};

#[derive(Debug, Clone, Default)]
Expand All @@ -19,58 +21,100 @@ impl FormatNodeRule<JsxExpressionAttributeValue> for FormatJsxExpressionAttribut
expression,
r_curly_token,
} = node.as_fields();
write!(
f,
[group(&format_with(|f| {
write!(f, [l_curly_token.format()])?;

let expression = expression.as_ref()?;
let expression = expression?;

// When the inner expression for a prop is an object, array, or call expression, we want to combine the
// delimiters of the expression (`{`, `}`, `[`, `]`, or `(`, `)`) with the delimiters of the JSX
// attribute (`{`, `}`), so that we don't end up with redundant indents. Therefore we do not
// soft indent the expression
//
// Good:
// ```jsx
// <ColorPickerPage
// colors={[
// "blue",
// "brown",
// "green",
// "orange",
// "purple",
// ]} />
// ```
//
// Bad:
// ```jsx
// <ColorPickerPage
// colors={
// [
// "blue",
// "brown",
// "green",
// "orange",
// "purple",
// ]
// } />
// ```
//
if matches!(
expression,
JsAnyExpression::JsObjectExpression(_)
| JsAnyExpression::JsArrayExpression(_)
| JsAnyExpression::JsCallExpression(_)
| JsAnyExpression::JsArrowFunctionExpression(_)
) {
write!(f, [expression.format()])?;
} else {
write!(f, [soft_block_indent(&expression.format())])?;
};
let should_inline = should_inline_jsx_expression(&expression, node.syntax().parent());

write!(f, [line_suffix_boundary(), r_curly_token.format()])
}))]
)
if should_inline {
write!(
f,
[
l_curly_token.format(),
expression.format(),
line_suffix_boundary(),
r_curly_token.format()
]
)
} else {
write!(
f,
[group(&format_args![
l_curly_token.format(),
soft_block_indent(&expression.format()),
line_suffix_boundary(),
r_curly_token.format()
])]
)
}
}
}

/// Tests if an expression inside of a [JsxExpressionChild] or [JsxExpressionAttributeValue] should be inlined.
/// Good:
/// ```jsx
/// <ColorPickerPage
/// colors={[
/// "blue",
/// "brown",
/// "green",
/// "orange",
/// "purple",
/// ]} />
/// ```
///
/// Bad:
/// ```jsx
/// <ColorPickerPage
/// colors={
/// [
/// "blue",
/// "brown",
/// "green",
/// "orange",
/// "purple",
/// ]
/// } />
/// ```
pub(crate) fn should_inline_jsx_expression(
expression: &JsAnyExpression,
parent: Option<JsSyntaxNode>,
) -> bool {
use JsAnyExpression::*;

if expression.syntax().has_comments_direct() {
return false;
}

match expression {
JsArrayExpression(_)
| JsObjectExpression(_)
| JsArrowFunctionExpression(_)
| JsCallExpression(_)
| JsNewExpression(_)
| JsImportCallExpression(_)
| ImportMeta(_)
| JsFunctionExpression(_)
| JsTemplate(_) => true,
JsAwaitExpression(await_expression) => match await_expression.argument() {
Ok(JsxTagExpression(argument)) => {
matches!(argument.tag(), Ok(JsxAnyTag::JsxElement(_)))
&& should_inline_jsx_expression(
&argument.into(),
Some(await_expression.syntax().clone()),
)
}
_ => false,
},
_ => {
// Don't indent conditional expressions only inside of children.
MichaReiser marked this conversation as resolved.
Show resolved Hide resolved
if matches!(expression, JsConditionalExpression(_))
|| JsAnyBinaryLikeExpression::can_cast(expression.syntax().kind())
{
parent.map_or(false, |parent| JsxChildList::can_cast(parent.kind()))
} else {
false
}
}
}
}
72 changes: 27 additions & 45 deletions crates/rome_js_formatter/src/jsx/auxiliary/expression_child.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
use crate::jsx::attribute::expression_attribute_value::should_inline_jsx_expression;
use crate::prelude::*;
use crate::prelude::{format_args, write};
use crate::utils::jsx::JsxSpace;
use rome_formatter::{group, CstFormatContext, FormatResult};
use rome_js_syntax::{
JsAnyExpression, JsAnyLiteralExpression, JsxExpressionChild, JsxExpressionChildFields,
};

use rome_formatter::{group, FormatResult};
use rome_js_syntax::{JsxExpressionChild, JsxExpressionChildFields};

#[derive(Debug, Clone, Default)]
pub struct FormatJsxExpressionChild;
Expand All @@ -17,47 +16,30 @@ impl FormatNodeRule<JsxExpressionChild> for FormatJsxExpressionChild {
r_curly_token,
} = node.as_fields();

let l_curly_token = l_curly_token?;
let r_curly_token = r_curly_token?;

// If the expression child is just a string literal with one space in it, it's a JSX space
if let Some(JsAnyExpression::JsAnyLiteralExpression(
JsAnyLiteralExpression::JsStringLiteralExpression(string_literal),
)) = &expression
{
let str_token = string_literal.value_token()?;
let trimmed_text = str_token.text_trimmed();
let should_inline = expression.as_ref().map_or(true, |expression| {
should_inline_jsx_expression(expression, node.syntax().parent())
});

let has_space_text = trimmed_text == "' '" || trimmed_text == "\" \"";
let no_trivia = !str_token.has_leading_non_whitespace_trivia()
&& !str_token.has_trailing_comments()
&& !l_curly_token.has_trailing_comments()
&& !r_curly_token.has_leading_non_whitespace_trivia();
let is_suppressed = f
.context()
.comments()
.is_suppressed(string_literal.syntax());

if has_space_text && no_trivia && !is_suppressed {
return write![
f,
[
format_removed(&l_curly_token),
format_replaced(&str_token, &JsxSpace::default()),
format_removed(&r_curly_token)
]
];
}
if should_inline {
write!(
f,
[
l_curly_token.format(),
expression.format(),
line_suffix_boundary(),
r_curly_token.format()
]
)
} else {
write!(
f,
[group(&format_args![
l_curly_token.format(),
soft_block_indent(&expression.format()),
line_suffix_boundary(),
r_curly_token.format()
])]
)
}

write![
f,
[group(&format_args![
l_curly_token.format(),
expression.format(),
line_suffix_boundary(),
r_curly_token.format()
])]
]
}
}
23 changes: 19 additions & 4 deletions crates/rome_js_formatter/src/jsx/auxiliary/spread_child.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,28 @@
use crate::prelude::*;
use rome_formatter::write;

use rome_js_syntax::JsxSpreadChild;
use rome_rowan::AstNode;
use rome_js_syntax::{JsxSpreadChild, JsxSpreadChildFields};

#[derive(Debug, Clone, Default)]
pub struct FormatJsxSpreadChild;

impl FormatNodeRule<JsxSpreadChild> for FormatJsxSpreadChild {
fn fmt_fields(&self, node: &JsxSpreadChild, formatter: &mut JsFormatter) -> FormatResult<()> {
format_verbatim_node(node.syntax()).fmt(formatter)
fn fmt_fields(&self, node: &JsxSpreadChild, f: &mut JsFormatter) -> FormatResult<()> {
let JsxSpreadChildFields {
l_curly_token,
dotdotdot_token,
expression,
r_curly_token,
} = node.as_fields();

write!(
f,
[
l_curly_token.format(),
dotdotdot_token.format(),
expression.format(),
r_curly_token.format()
]
)
}
}
Loading