refactor(rome_js_formatter): Remove unnecessary groups from `format_s…

…eparated` `format_separated` used to group all elements by default. This is often undesired and also comes with a performance cost (printer needs to test if the elements fit). This PR changes the default of `format_separated` so that it doesn't group the elements except if enabled explicitly using `nodes_grouped`. This PR further fixes the formatting of named import specifiers to more closely match Prettier's foramtting.
rome · Jun 23, 2022 · f8074de · f8074de
1 parent ffe1f92
commit f8074de
Show file tree

Hide file tree

Showing 15 changed files with 41 additions and 74 deletions.
diff --git a/crates/rome_formatter/src/printer/mod.rs b/crates/rome_formatter/src/printer/mod.rs
@@ -327,11 +327,7 @@ impl<'a> Printer<'a> {
         queue.extend(
             line_break
                 .into_iter()
-                .chain(self.state.line_suffixes.drain(..).map(move |mut call| {
-                    // Overwrite the arguments for the PrintElementCalls in the queue with the current arguments
-                    call.args = args;
-                    call
-                }))
+                .chain(self.state.line_suffixes.drain(..))
                 .chain(once(call_self)),
         );
     }

diff --git a/crates/rome_js_formatter/src/js/expressions/call_arguments.rs b/crates/rome_js_formatter/src/js/expressions/call_arguments.rs
@@ -82,10 +82,7 @@ impl FormatNodeRule<JsCallArguments> for FormatJsCallArguments {
             // We also disallow the trailing separator, we are interested in doing it manually.
             let separated: Vec<_> = args
                 .format_separated(JsSyntaxKind::COMMA)
-                .with_options(
-                    FormatSeparatedOptions::default()
-                        .with_trailing_separator(TrailingSeparator::Omit),
-                )
+                .with_trailing_separator(TrailingSeparator::Omit)
                 .map(|e| e.memoized())
                 .collect();
 
@@ -191,11 +188,10 @@ impl FormatNodeRule<JsCallArguments> for FormatJsCallArguments {
                         l_paren,
                         l_trailing_trivia,
                         &soft_block_indent(&format_with(|f| {
-                            let separated =
-                                args.format_separated(JsSyntaxKind::COMMA).with_options(
-                                    FormatSeparatedOptions::default()
-                                        .with_trailing_separator(TrailingSeparator::Omit),
-                                );
+                            let separated = args
+                                .format_separated(JsSyntaxKind::COMMA)
+                                .with_trailing_separator(TrailingSeparator::Omit)
+                                .nodes_grouped();
                             write_arguments_multi_line(separated, f)
                         }),),
                         r_leading_trivia,

diff --git a/crates/rome_js_formatter/src/js/lists/call_argument_list.rs b/crates/rome_js_formatter/src/js/lists/call_argument_list.rs
@@ -17,10 +17,9 @@ impl FormatRule<JsCallArgumentList> for FormatJsCallArgumentList {
         write!(
             f,
             [&group_elements(&soft_block_indent(&format_with(|f| {
-                let separated = node.format_separated(JsSyntaxKind::COMMA).with_options(
-                    FormatSeparatedOptions::default()
-                        .with_trailing_separator(TrailingSeparator::Omit),
-                );
+                let separated = node
+                    .format_separated(JsSyntaxKind::COMMA)
+                    .with_trailing_separator(TrailingSeparator::Omit);
                 write_arguments_multi_line(separated, f)
             })))]
         )

diff --git a/crates/rome_js_formatter/src/js/lists/object_assignment_pattern_property_list.rs b/crates/rome_js_formatter/src/js/lists/object_assignment_pattern_property_list.rs
@@ -32,9 +32,10 @@ impl FormatRule<JsObjectAssignmentPatternPropertyList>
         };
 
         f.join_with(&soft_line_break_or_space())
-            .entries(node.format_separated(JsSyntaxKind::COMMA).with_options(
-                FormatSeparatedOptions::default().with_trailing_separator(trailing_separator),
-            ))
+            .entries(
+                node.format_separated(JsSyntaxKind::COMMA)
+                    .with_trailing_separator(trailing_separator),
+            )
             .finish()
     }
 }
diff --git a/crates/rome_js_formatter/src/js/lists/object_binding_pattern_property_list.rs b/crates/rome_js_formatter/src/js/lists/object_binding_pattern_property_list.rs
@@ -30,9 +30,10 @@ impl FormatRule<JsObjectBindingPatternPropertyList> for FormatJsObjectBindingPat
         };
 
         f.join_with(&soft_line_break_or_space())
-            .entries(node.format_separated(JsSyntaxKind::COMMA).with_options(
-                FormatSeparatedOptions::default().with_trailing_separator(trailing_separator),
-            ))
+            .entries(
+                node.format_separated(JsSyntaxKind::COMMA)
+                    .with_trailing_separator(trailing_separator),
+            )
             .finish()
     }
 }
diff --git a/crates/rome_js_formatter/src/js/lists/parameter_list.rs b/crates/rome_js_formatter/src/js/lists/parameter_list.rs
@@ -21,9 +21,10 @@ impl FormatRule<JsParameterList> for FormatJsParameterList {
         };
 
         f.join_with(&soft_line_break_or_space())
-            .entries(node.format_separated(JsSyntaxKind::COMMA).with_options(
-                FormatSeparatedOptions::default().with_trailing_separator(trailing_separator),
-            ))
+            .entries(
+                node.format_separated(JsSyntaxKind::COMMA)
+                    .with_trailing_separator(trailing_separator),
+            )
             .finish()
     }
 }
diff --git a/crates/rome_js_formatter/src/js/module/named_import_specifier.rs b/crates/rome_js_formatter/src/js/module/named_import_specifier.rs
@@ -24,9 +24,9 @@ impl FormatNodeRule<JsNamedImportSpecifier> for FormatJsNamedImportSpecifier {
             f,
             [
                 name.format(),
-                soft_line_break_or_space(),
+                space_token(),
                 as_token.format(),
-                soft_line_break_or_space(),
+                space_token(),
                 local_name.format()
             ]
         ]

diff --git a/crates/rome_js_formatter/src/separated.rs b/crates/rome_js_formatter/src/separated.rs
@@ -25,7 +25,7 @@ where
         let node = self.element.node()?;
         let separator = self.element.trailing_separator()?;
 
-        if self.options.disable_group_nodes {
+        if !self.options.nodes_grouped {
             node.format().fmt(f)?;
         } else {
             group_elements(&node.format()).fmt(f)?;
@@ -107,15 +107,9 @@ where
         }
     }
 
-    pub fn with_options(mut self, options: FormatSeparatedOptions) -> Self {
-        self.options = options;
-        self
-    }
-
-    /// Sets whatever the nodes should be wrapped by a `group_elements` (default: `true` for compatibility
-    /// reasons).
-    pub fn group_nodes(mut self, group_nodes: bool) -> Self {
-        self.options.disable_group_nodes = !group_nodes;
+    /// Wraps every node inside of a group
+    pub fn nodes_grouped(mut self) -> Self {
+        self.options.nodes_grouped = true;
         self
     }
 
@@ -210,18 +204,5 @@ impl Default for TrailingSeparator {
 pub struct FormatSeparatedOptions {
     trailing_separator: TrailingSeparator,
     group_id: Option<GroupId>,
-    disable_group_nodes: bool,
-}
-
-impl FormatSeparatedOptions {
-    pub fn with_trailing_separator(mut self, separator: TrailingSeparator) -> Self {
-        self.trailing_separator = separator;
-        self
-    }
-
-    #[allow(unused)]
-    pub fn with_group_id(mut self, group_id: Option<GroupId>) -> Self {
-        self.group_id = group_id;
-        self
-    }
+    nodes_grouped: bool,
 }
diff --git a/crates/rome_js_formatter/src/ts/lists/enum_member_list.rs b/crates/rome_js_formatter/src/ts/lists/enum_member_list.rs
@@ -16,7 +16,7 @@ impl FormatRule<TsEnumMemberList> for FormatTsEnumMemberList {
         } else {
             soft_line_break_or_space()
         })
-        .entries(node.format_separated(JsSyntaxKind::COMMA))
+        .entries(node.format_separated(JsSyntaxKind::COMMA).nodes_grouped())
         .finish()
     }
 }
diff --git a/crates/rome_js_formatter/src/ts/lists/tuple_type_element_list.rs b/crates/rome_js_formatter/src/ts/lists/tuple_type_element_list.rs
@@ -9,7 +9,7 @@ impl FormatRule<TsTupleTypeElementList> for FormatTsTupleTypeElementList {
 
     fn fmt(&self, node: &TsTupleTypeElementList, f: &mut JsFormatter) -> FormatResult<()> {
         f.join_with(&soft_line_break_or_space())
-            .entries(node.format_separated(JsSyntaxKind::COMMA))
+            .entries(node.format_separated(JsSyntaxKind::COMMA).nodes_grouped())
             .finish()
     }
 }
diff --git a/crates/rome_js_formatter/src/ts/lists/type_argument_list.rs b/crates/rome_js_formatter/src/ts/lists/type_argument_list.rs
@@ -11,8 +11,7 @@ impl FormatRule<TsTypeArgumentList> for FormatTsTypeArgumentList {
         f.join_with(&soft_line_break_or_space())
             .entries(
                 node.format_separated(JsSyntaxKind::COMMA)
-                    .with_trailing_separator(TrailingSeparator::Disallowed)
-                    .group_nodes(false),
+                    .with_trailing_separator(TrailingSeparator::Disallowed),
             )
             .finish()
     }

diff --git a/crates/rome_js_formatter/src/ts/lists/type_list.rs b/crates/rome_js_formatter/src/ts/lists/type_list.rs
@@ -12,8 +12,7 @@ impl FormatRule<TsTypeList> for FormatTsTypeList {
         f.join_with(&soft_line_break_or_space())
             .entries(
                 node.format_separated(JsSyntaxKind::COMMA)
-                    .with_trailing_separator(TrailingSeparator::Disallowed)
-                    .group_nodes(false),
+                    .with_trailing_separator(TrailingSeparator::Disallowed),
             )
             .finish()
     }

diff --git a/crates/rome_js_formatter/src/ts/lists/type_parameter_list.rs b/crates/rome_js_formatter/src/ts/lists/type_parameter_list.rs
@@ -22,9 +22,10 @@ impl FormatRule<TsTypeParameterList> for FormatTsTypeParameterList {
         };
 
         f.join_with(&soft_line_break_or_space())
-            .entries(node.format_separated(JsSyntaxKind::COMMA).with_options(
-                FormatSeparatedOptions::default().with_trailing_separator(trailing_separator),
-            ))
+            .entries(
+                node.format_separated(JsSyntaxKind::COMMA)
+                    .with_trailing_separator(trailing_separator),
+            )
             .finish()
     }
 }
diff --git a/crates/rome_js_formatter/tests/specs/prettier/js/comments/export-and-import.js.snap b/crates/rome_js_formatter/tests/specs/prettier/js/comments/export-and-import.js.snap
@@ -51,9 +51,7 @@ export {
 
 import {
   foo,
-  bar
-  as // comment
-  baz,
+  bar as baz, // comment
 } from "foo";
 
 ```

diff --git a/crates/rome_js_formatter/tests/specs/prettier/js/import/comments.js.snap b/crates/rome_js_formatter/tests/specs/prettier/js/import/comments.js.snap
@@ -58,29 +58,24 @@ import {
   a //comment1
   //comment2
   //comment3
-  as
-  b,
+  as b,
 } from "";
 
 import {
-  a
-  as //comment1
+  a as //comment1
   //comment2
   //comment3
   b1,
 } from "";
 
 import {
-  a
-  as //comment2 //comment1
+  a as //comment2 //comment1
   //comment3
   b2,
 } from "";
 
 import {
-  a
-  as //comment3 //comment2 //comment1
-  b3,
+  a as b3, //comment3 //comment2 //comment1
 } from "";
 
 import {