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

Commit

Permalink
feat(rome_js_formatter): Conditional JSX Chain layout
Browse files Browse the repository at this point in the history 
This PR implements Prettier's conditional JSX chain layout.

```
abcdefgh ? (
	<Element>
		<Sub />
		<Sub />
	</Element>
) : (
	<Element2>
		<Sub />
		<Sub />
	</Element2>
);
```

That parenthesizes the `consequent` and `alternate` except for `null`, `undefined`, and nested conditionals in the alternate.

This PR further removes the `Default` implementation from `JsFormatOptions` because passing a `SourceType` that matches with the type of the AST is mandatory to get correct formatting results.

## Tests

I verified the prettier snapshots and added our own tests to cover the exceptions of `null`, `undefined` and nested alternates that don't get parenthesized.
  • Loading branch information
@MichaReiser
MichaReiser committed Sep 2, 2022
1 parent 9cee4fa commit 57b8c32
Show file tree
Hide file tree
Showing 10 changed files with 271 additions and 360 deletions.
14 changes: 7 additions & 7 deletions crates/rome_js_formatter/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -631,7 +631,7 @@ while(

let tree = parse_script(input, 0);
let result = format_range(
JsFormatOptions::default().with_indent_style(IndentStyle::Space(4)),
JsFormatOptions::new(SourceType::js_script()).with_indent_style(IndentStyle::Space(4)),
&tree.syntax(),
TextRange::new(range_start, range_end),
);
Expand Down Expand Up @@ -663,7 +663,7 @@ function() {

let tree = parse_script(input, 0);
let result = format_range(
JsFormatOptions::default().with_indent_style(IndentStyle::Space(4)),
JsFormatOptions::new(SourceType::js_script()).with_indent_style(IndentStyle::Space(4)),
&tree.syntax(),
TextRange::new(range_start, range_end),
);
Expand Down Expand Up @@ -694,7 +694,7 @@ function() {

let tree = parse_script(input, 0);
let result = format_range(
JsFormatOptions::default().with_indent_style(IndentStyle::Space(4)),
JsFormatOptions::new(SourceType::js_script()).with_indent_style(IndentStyle::Space(4)),
&tree.syntax(),
TextRange::new(range_start, range_end),
);
Expand All @@ -713,7 +713,7 @@ function() {

let tree = parse_script(input, 0);
let result = format_range(
JsFormatOptions::default().with_indent_style(IndentStyle::Space(4)),
JsFormatOptions::new(SourceType::js_script()).with_indent_style(IndentStyle::Space(4)),
&tree.syntax(),
TextRange::new(range_start, range_end),
);
Expand All @@ -735,7 +735,7 @@ function() {

let tree = parse_script(input, 0);
let result = format_range(
JsFormatOptions::default().with_indent_style(IndentStyle::Space(4)),
JsFormatOptions::new(SourceType::js_script()).with_indent_style(IndentStyle::Space(4)),
&tree.syntax(),
TextRange::new(range_start, range_end),
);
Expand All @@ -756,7 +756,7 @@ const getIconEngagementTypeFrom = (engagementTypes: Array<EngagementType>) =>
"#;
let syntax = SourceType::tsx();
let tree = parse(src, 0, syntax);
let options = JsFormatOptions::default();
let options = JsFormatOptions::new(syntax);

let result = format_node(options.clone(), &tree.syntax())
.unwrap()
Expand All @@ -782,7 +782,7 @@ const getIconEngagementTypeFrom = (engagementTypes: Array<EngagementType>) =>
let tree = parse(src, 0, syntax);

let result = format_range(
JsFormatOptions::default(),
JsFormatOptions::new(syntax),
&tree.syntax(),
TextRange::new(TextSize::from(0), TextSize::of(src) + TextSize::from(5)),
);
Expand Down
230 changes: 204 additions & 26 deletions crates/rome_js_formatter/src/utils/conditional.rs
View file
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
use crate::prelude::*;
use rome_formatter::write;
use rome_formatter::{write, FormatContext};

use rome_js_syntax::{
JsAnyExpression, JsAssignmentExpression, JsCallExpression, JsComputedMemberExpression,
JsConditionalExpression, JsInitializerClause, JsNewExpression, JsReturnStatement,
JsStaticMemberExpression, JsSyntaxKind, JsSyntaxNode, JsSyntaxToken, JsThrowStatement,
JsUnaryExpression, JsYieldArgument, TsAsExpression, TsConditionalType,
JsUnaryExpression, JsYieldArgument, SourceType, TsAsExpression, TsConditionalType,
TsNonNullAssertionExpression, TsType,
};
use rome_rowan::{declare_node_union, match_ast, AstNode, SyntaxResult};
Expand All @@ -20,7 +20,7 @@ impl Format<JsFormatContext> for JsAnyConditional {
let consequent = self.consequent()?;
let alternate = self.alternate()?;

let layout = self.layout();
let layout = self.layout(f.context().options().source_type());

let format_consequent_and_alternate = format_with(|f| {
write!(
Expand Down Expand Up @@ -78,8 +78,23 @@ impl Format<JsFormatContext> for JsAnyConditional {
});

let format_tail_with_indent = format_with(|f: &mut JsFormatter| {
if layout.is_jsx_chain() {
write!(
f,
[
space(),
self.question_mark_token().format(),
space(),
format_jsx_chain_consequent(&consequent),
space(),
self.colon_token().format(),
space(),
format_jsx_chain_alternate(&alternate)
]
)
}
// Add an extra level of indent to nested consequences.
if layout.is_nested_consequent() {
else if layout.is_nested_consequent() {
// if f.context().indent_style().is_tab() {
// This may look silly but the `dedent` is to remove the outer `align` added by the parent's formatting of the consequent.
// The `indent` is necessary to indent the content by one level with a tab.
Expand All @@ -104,14 +119,15 @@ impl Format<JsFormatContext> for JsAnyConditional {

// Indent the `consequent` and `alternate` **only if** this is the root conditional expression
// OR this is the `test` of a conditional expression.
if layout.is_root() || layout.is_nested_test() {
if !layout.is_jsx_chain() && (layout.is_root() || layout.is_nested_test()) {
write!(f, [indent(&format_tail_with_indent)])?;
} else {
// Don't indent for nested `alternate`s or `consequence`s
write!(f, [format_tail_with_indent])?;
}

let break_closing_parentheses = self.is_parent_static_member_expression(&layout);
let break_closing_parentheses =
!layout.is_jsx_chain() && self.is_parent_static_member_expression(&layout);

// Add a soft line break in front of the closing `)` in case the parent is a static member expression
// ```
Expand Down Expand Up @@ -199,12 +215,12 @@ enum ConditionalLayout {
///
/// ```javascript
/// outerCondition
// ? consequent
// : nestedAlternate +
// binary + // <- notice how the content is aligned to the `: `
// ? consequentOfnestedAlternate
// : alternateOfNestedAlternate;
// ```
/// ? consequent
/// : nestedAlternate +
/// binary + // <- notice how the content is aligned to the `: `
/// ? consequentOfnestedAlternate
/// : alternateOfNestedAlternate;
/// ```
NestedAlternate { parent: JsAnyConditional },

/// Conditional that is the `test` of another conditional.
Expand All @@ -226,10 +242,10 @@ enum ConditionalLayout {
///
/// ```javascript
/// condition1
// ? condition2
// ? consequent2 // <-- consequent and alternate gets indented
// : alternate2
// : alternate1;
/// ? condition2
/// ? consequent2 // <-- consequent and alternate gets indented
/// : alternate2
/// : alternate1;
/// ```
NestedConsequent { parent: JsAnyConditional },

Expand All @@ -242,9 +258,41 @@ enum ConditionalLayout {
/// The closest ancestor that isn't a parenthesized node.
parent: Option<JsSyntaxNode>,
},

/// A [JsConditionalExpression] that itself or any of its parent's [JsConditionalExpression] have a a [JsxTagExpression]
/// as its [`test`](JsConditionalExpression::test), [`consequent`](JsConditionalExpression::consequent) or [`alternate`](JsConditionalExpression::alternate).
///
/// Parenthesizes the `consequent` and `alternate` if it the group breaks except if the expressions are
/// * `null`
/// * `undefined`
/// * or a nested [JsConditionalExpression] in the alternate branch
///
/// ```javascript
/// abcdefgh? (
/// <Element>
/// <Sub />
/// <Sub />
/// </Element>
/// ) : (
/// <Element2>
/// <Sub />
/// <Sub />
/// </Element2>
/// );
/// ```
JsxChain {
/// The direct parent of the node. May be another conditional if it is part of a chain
parent: Option<JsSyntaxNode>,

is_nested_test: bool,
},
}

impl ConditionalLayout {
const fn is_jsx_chain(&self) -> bool {
matches!(self, ConditionalLayout::JsxChain { .. })
}

const fn is_root(&self) -> bool {
matches!(self, ConditionalLayout::Root { .. })
}
Expand All @@ -255,12 +303,21 @@ impl ConditionalLayout {
ConditionalLayout::NestedAlternate { parent }
| ConditionalLayout::NestedTest { parent }
| ConditionalLayout::NestedConsequent { parent } => Some(parent.syntax()),
ConditionalLayout::Root { parent } => parent.as_ref(),
ConditionalLayout::Root { parent } | ConditionalLayout::JsxChain { parent, .. } => {
parent.as_ref()
}
}
}

const fn is_nested_test(&self) -> bool {
matches!(self, ConditionalLayout::NestedTest { .. })
matches!(
self,
ConditionalLayout::NestedTest { .. }
| ConditionalLayout::JsxChain {
is_nested_test: true,
..
}
)
}

const fn is_nested_alternate(&self) -> bool {
Expand All @@ -273,13 +330,48 @@ impl ConditionalLayout {
}

impl JsAnyConditional {
fn layout(&self) -> ConditionalLayout {
let resolved_parent = match self {
JsAnyConditional::JsConditionalExpression(conditional) => conditional.syntax().parent(),
JsAnyConditional::TsConditionalType(ty) => ty.syntax().parent(),
};
fn layout(&self, source_type: SourceType) -> ConditionalLayout {
// Tests if the conditional expression is a JSX chain. This is rather expensive
// as it first needs to find the outer most conditional expression and then traverses
// all conditional expressions in the sub-tree to test if ANY contains a `JsxTagExpression`.
// Let's only do this if JSX is enabled
if source_type.variant().is_jsx() {
if let JsAnyConditional::JsConditionalExpression(conditional) = self {
let mut outer_most_conditional = conditional.clone();

while let Some(parent) = outer_most_conditional.syntax().parent() {
if let Some(conditional) = JsConditionalExpression::cast(parent) {
outer_most_conditional = conditional;
} else {
break;
}
}

if is_jsx_tag_expression(conditional.test())
|| is_jsx_tag_expression(conditional.alternate())
|| is_jsx_tag_expression(conditional.consequent())
|| is_jsx_conditional_chain(outer_most_conditional)
{
let parent = self.syntax().parent();
let is_nested_test = parent.as_ref().map_or(false, |parent| {
if parent.kind() == self.syntax().kind() {
Self::unwrap_cast(parent.clone()).is_test(self.syntax())
} else {
false
}
});

return ConditionalLayout::JsxChain {
parent: self.syntax().parent(),
is_nested_test,
};
}
}
}

let parent = self.syntax().parent();

let parent = match resolved_parent {
let parent = match parent {
None => return ConditionalLayout::Root { parent: None },
Some(parent) => parent,
};
Expand Down Expand Up @@ -421,8 +513,7 @@ impl JsAnyConditional {
let mut parent = None;
let mut expression = JsAnyExpression::from(conditional.clone());

// This tries to find the start of a member chain by iterating over all ancestors of the conditional
// expression, while skipping parenthesized expression.
// This tries to find the start of a member chain by iterating over all ancestors of the conditional.
// The iteration "breaks" as soon as a non-member-chain node is found.
for ancestor in ancestors {
let ancestor = match_ast! {
Expand Down Expand Up @@ -574,3 +665,90 @@ impl JsAnyConditional {
}
}
}

/// Returns `true` if the passed `expression` is a [JsxTagExpression]
fn is_jsx_tag_expression(expression: SyntaxResult<JsAnyExpression>) -> bool {
expression.map_or(false, |expression| {
matches!(expression, JsAnyExpression::JsxTagExpression(_))
})
}

fn is_jsx_conditional_chain(outer_most: JsConditionalExpression) -> bool {
fn recurse(expression: SyntaxResult<JsAnyExpression>) -> bool {
match expression {
Ok(JsAnyExpression::JsConditionalExpression(conditional)) => {
is_jsx_conditional_chain(conditional)
}
_ => false,
}
}

recurse(outer_most.test())
|| recurse(outer_most.consequent())
|| recurse(outer_most.alternate())
}

fn format_jsx_chain_consequent(expression: &ExpressionOrType) -> FormatJsxChainExpression {
let expression = match expression {
ExpressionOrType::JsAnyExpression(expression) => expression,
ExpressionOrType::TsType(_) => {
panic!("JSX Chain formatting not supported for typescript types")
}
};

FormatJsxChainExpression {
expression,
alternate: false,
}
}

fn format_jsx_chain_alternate(alternate: &ExpressionOrType) -> FormatJsxChainExpression {
let alternate = match alternate {
ExpressionOrType::JsAnyExpression(expression) => expression,
ExpressionOrType::TsType(_) => {
panic!("JSX Chain formatting not supported for typescript types")
}
};

FormatJsxChainExpression {
expression: alternate,
alternate: true,
}
}

/// Wraps all expressions in parentheses if they break EXCEPT
/// * Nested conditionals in the alterante
/// * `null`
/// * `undefined`
struct FormatJsxChainExpression<'a> {
expression: &'a JsAnyExpression,
alternate: bool,
}

impl Format<JsFormatContext> for FormatJsxChainExpression<'_> {
fn fmt(&self, f: &mut Formatter<JsFormatContext>) -> FormatResult<()> {
use JsAnyExpression::*;

let no_wrap = match self.expression {
JsIdentifierExpression(identifier) if identifier.name()?.is_undefined() => true,
JsAnyLiteralExpression(
rome_js_syntax::JsAnyLiteralExpression::JsNullLiteralExpression(_),
) => true,
JsConditionalExpression(_) if self.alternate => true,
_ => false,
};

if no_wrap {
write!(f, [self.expression.format()])
} else {
write!(
f,
[
if_group_breaks(&text("(")),
soft_block_indent(&self.expression.format()),
if_group_breaks(&text(")"))
]
)
}
}
}
2 changes: 1 addition & 1 deletion crates/rome_js_formatter/tests/prettier_tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ fn test_snapshot(input: &'static str, _: &str, _: &str, _: &str) {
let has_errors = parsed.has_errors();
let syntax = parsed.syntax();

let options = JsFormatOptions::default().with_indent_style(IndentStyle::Space(2));
let options = JsFormatOptions::new(source_type).with_indent_style(IndentStyle::Space(2));

let result = match (range_start_index, range_end_index) {
(Some(start), Some(end)) => {
Expand Down
Loading

0 comments on commit 57b8c32

Please sign in to comment.