feat(rome_js_formatter): Break long object literal members over two l…

…ines rome#2425
denbezrukov · Jun 1, 2022 · 8cd8bf8 · 8cd8bf8
1 parent 4e6f35f
commit 8cd8bf8
Show file tree

Hide file tree

Showing 17 changed files with 351 additions and 170 deletions.
diff --git a/crates/rome_js_formatter/src/js/objects/property_object_member.rs b/crates/rome_js_formatter/src/js/objects/property_object_member.rs
@@ -1,8 +1,16 @@
 use crate::prelude::*;
 
 use crate::FormatNodeFields;
+use rome_js_syntax::JsAnyExpression;
+use rome_js_syntax::JsAnyLiteralExpression;
+use rome_js_syntax::JsCallExpressionFields;
+use rome_js_syntax::JsConditionalExpressionFields;
 use rome_js_syntax::JsPropertyObjectMember;
 use rome_js_syntax::JsPropertyObjectMemberFields;
+use rome_js_syntax::JsSyntaxKind;
+use rome_js_syntax::JsSyntaxNode;
+use rome_rowan::SyntaxResult;
+use rome_rowan::TextSize;
 
 impl FormatNodeFields<JsPropertyObjectMember> for FormatNodeRule<JsPropertyObjectMember> {
     fn format_fields(
@@ -14,10 +22,175 @@ impl FormatNodeFields<JsPropertyObjectMember> for FormatNodeRule<JsPropertyObjec
             colon_token,
             value,
         } = node.as_fields();
+        let layout = get_object_member_layout(formatter, node)?;
 
-        let key = name.format();
-        let colon = colon_token.format();
-        let value = value.format();
-        formatted![formatter, [key, colon, space_token(), value]]
+        let formatted = match layout {
+            PropertyObjectMemberLayoutMode::Fluid => {
+                let group_id = formatter.group_id("property_object_member");
+
+                let value = formatted![formatter, [value.format()]]?;
+                formatted![
+                    formatter,
+                    [
+                        group_elements(formatted![formatter, [name.format()]]?),
+                        colon_token.format(),
+                        group_elements_with_options(
+                            indent(soft_line_break_or_space()),
+                            GroupElementsOptions {
+                                group_id: Some(group_id)
+                            }
+                        ),
+                        line_suffix_boundary(),
+                        if_group_with_id_breaks(indent(value.clone()), group_id),
+                        if_group_with_id_fits_on_line(value, group_id),
+                    ]
+                ]
+            }
+            PropertyObjectMemberLayoutMode::BreakAfterOperator => formatted![
+                formatter,
+                [
+                    group_elements(formatted![formatter, [name.format()]]?),
+                    colon_token.format(),
+                    space_token(),
+                    group_elements(formatted![
+                        formatter,
+                        [indent(soft_line_break_or_space()), value.format()]
+                    ]?),
+                ]
+            ],
+            PropertyObjectMemberLayoutMode::NeverBreakAfterOperator => formatted![
+                formatter,
+                [
+                    group_elements(formatted![formatter, [name.format()]]?),
+                    colon_token.format(),
+                    space_token(),
+                    value.format()
+                ]
+            ],
+        };
+
+        Ok(group_elements(formatted?))
+    }
+}
+
+enum PropertyObjectMemberLayoutMode {
+    Fluid,
+    BreakAfterOperator,
+    NeverBreakAfterOperator,
+}
+
+fn get_object_member_layout(
+    formatter: &Formatter<JsFormatOptions>,
+    node: &JsPropertyObjectMember,
+) -> SyntaxResult<PropertyObjectMemberLayoutMode> {
+    let JsPropertyObjectMemberFields {
+        name,
+        colon_token: _,
+        value,
+    } = node.as_fields();
+
+    let name = name?;
+    let value = value?;
+
+    let value = match value {
+        JsAnyExpression::TsAsExpression(expression) => expression.expression()?,
+        _ => value,
+    };
+
+    const MIN_OVERLAP_FOR_BREAK: u8 = 3;
+
+    let text_width_for_break = (formatter.options().tab_width() + MIN_OVERLAP_FOR_BREAK) as u32;
+    let has_short_key = name.range().len() < TextSize::from(text_width_for_break);
+
+    if is_break_after_operator(&value, has_short_key)? {
+        return Ok(PropertyObjectMemberLayoutMode::BreakAfterOperator);
+    }
+
+    if is_never_break_after_operator(&value, has_short_key)? {
+        return Ok(PropertyObjectMemberLayoutMode::NeverBreakAfterOperator);
+    }
+
+    Ok(PropertyObjectMemberLayoutMode::Fluid)
+}
+
+fn is_break_after_operator(value: &JsAnyExpression, has_short_key: bool) -> SyntaxResult<bool> {
+    if is_binaryish_expression(value.syntax()) {
+        return Ok(true);
+    }
+
+    if matches!(value, JsAnyExpression::JsSequenceExpression(_)) {
+        return Ok(true);
     }
+
+    if let JsAnyExpression::JsConditionalExpression(conditional) = &value {
+        let JsConditionalExpressionFields {
+            test,
+            question_mark_token: _,
+            consequent: _,
+            colon_token: _,
+            alternate: _,
+        } = conditional.as_fields();
+
+        if is_binaryish_expression(test?.syntax()) {
+            return Ok(true);
+        }
+    }
+
+    if has_short_key {
+        return Ok(false);
+    }
+
+    if let JsAnyExpression::JsAnyLiteralExpression(
+        JsAnyLiteralExpression::JsStringLiteralExpression(_),
+    ) = &value
+    {
+        return Ok(true);
+    }
+
+    Ok(false)
+}
+
+fn is_never_break_after_operator(
+    value: &JsAnyExpression,
+    has_short_key: bool,
+) -> SyntaxResult<bool> {
+    if let JsAnyExpression::JsCallExpression(call_expression) = &value {
+        let JsCallExpressionFields {
+            callee,
+            optional_chain_token: _,
+            type_arguments: _,
+            arguments: _,
+        } = call_expression.as_fields();
+
+        if callee?.syntax().kind() == JsSyntaxKind::REQUIRE_KW {
+            return Ok(true);
+        }
+    }
+
+    if has_short_key {
+        return Ok(true);
+    }
+
+    if matches!(
+        value,
+        JsAnyExpression::JsClassExpression(_)
+            | JsAnyExpression::JsTemplate(_)
+            | JsAnyExpression::JsAnyLiteralExpression(
+                JsAnyLiteralExpression::JsBooleanLiteralExpression(_),
+            )
+            | JsAnyExpression::JsAnyLiteralExpression(
+                JsAnyLiteralExpression::JsNumberLiteralExpression(_)
+            )
+    ) {
+        return Ok(true);
+    }
+
+    Ok(false)
+}
+
+fn is_binaryish_expression(node: &JsSyntaxNode) -> bool {
+    matches!(
+        node.kind(),
+        JsSyntaxKind::JS_BINARY_EXPRESSION | JsSyntaxKind::JS_LOGICAL_EXPRESSION
+    )
 }
diff --git a/crates/rome_js_formatter/tests/specs/js/module/expression/sequence_expression.js.snap b/crates/rome_js_formatter/tests/specs/js/module/expression/sequence_expression.js.snap
@@ -1,6 +1,5 @@
 ---
 source: crates/rome_js_formatter/tests/spec_test.rs
-assertion_line: 244
 expression: sequence_expression.js
 ---
 # Input
@@ -74,15 +73,16 @@ function a() {
 }
 
 const object = {
-	something: (
-		____________first,
-		____________second,
-		____________third,
-		____________third,
-		____________third,
-		____________third,
-		____________third
-	),
+	something:
+		(
+			____________first,
+			____________second,
+			____________third,
+			____________third,
+			____________third,
+			____________third,
+			____________third
+		),
 };
 
 aLongIdentifierName,

diff --git a/crates/rome_js_formatter/tests/specs/jsx/attributes.jsx.snap b/crates/rome_js_formatter/tests/specs/jsx/attributes.jsx.snap
@@ -1,8 +1,6 @@
 ---
 source: crates/rome_js_formatter/tests/spec_test.rs
-assertion_line: 242
 expression: attributes.jsx
-
 ---
 # Input
 
@@ -67,7 +65,8 @@ Quote style: Double Quotes
 		fontSize: 12,
 		height: "40vh",
 		overflowY: "scroll",
-		fontFamily: "ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
+		fontFamily:
+			"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
 	}}
 />;
 
@@ -89,7 +88,8 @@ Quote style: Double Quotes
 		fontSize: 12,
 		height: "50vh",
 		overflowY: "scroll",
-		fontFamily: "ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
+		fontFamily:
+			"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
 	}}
 />;
 
@@ -115,6 +115,6 @@ let a = <test
 
 ## Lines exceeding width of 80 characters
 
-    9: 		fontFamily: "ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
-   31: 		fontFamily: "ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
+   10: 			"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
+   33: 			"ui-monospace,SFMono-Regular,SF Mono,Consolas,Liberation Mono,Menlo,monospace",
 
diff --git a/crates/rome_js_formatter/tests/specs/prettier/js/assignment/issue-7572.js.snap b/crates/rome_js_formatter/tests/specs/prettier/js/assignment/issue-7572.js.snap
@@ -17,14 +17,11 @@ const t = {
 ```js
 const t = {
   "hello": world(),
-  "this-is-a-very-long-key-and-the-assignment-should-be-put-on-the-next-line": orMaybeIAmMisunderstandingAndIHaveSetSomethingWrongInMyConfig(),
+  "this-is-a-very-long-key-and-the-assignment-should-be-put-on-the-next-line":
+    orMaybeIAmMisunderstandingAndIHaveSetSomethingWrongInMyConfig(),
   "can-someone-explain": this(),
 };
 
 ```
 
-# Lines exceeding max width of 80 characters
-```
-    3:   "this-is-a-very-long-key-and-the-assignment-should-be-put-on-the-next-line": orMaybeIAmMisunderstandingAndIHaveSetSomethingWrongInMyConfig(),
-```
 
diff --git a/.../rome_js_formatter/tests/specs/prettier/js/binary-expressions/inline-object-array.js.snap b/.../rome_js_formatter/tests/specs/prettier/js/binary-expressions/inline-object-array.js.snap
@@ -193,13 +193,15 @@ const create2 = () => {
 };
 
 const obj = {
-  state: shouldHaveState &&
+  state:
+    shouldHaveState &&
     stateIsOK &&
     {
       loadState: LOADED,
       opened: false,
     },
-  loadNext: (stateIsOK && hasNext) || {
+  loadNext:
+    (stateIsOK && hasNext) || {
     skipNext: true,
   },
   loaded: true,

diff --git a/...es/rome_js_formatter/tests/specs/prettier/js/functional-composition/ramda_compose.js.snap b/...es/rome_js_formatter/tests/specs/prettier/js/functional-composition/ramda_compose.js.snap
@@ -96,10 +96,8 @@ followersForUser(
 // Followers: ["STEVE","SUZY"]
 
 const mapStateToProps = (state) => ({
-  users: R.compose(
-    R.filter(R.propEq("status", "active")),
-    R.values,
-  )(state.users),
+  users:
+    R.compose(R.filter(R.propEq("status", "active")), R.values)(state.users),
 });
 
 ```

diff --git a/...es/rome_js_formatter/tests/specs/prettier/js/last-argument-expansion/break-parent.js.snap b/...es/rome_js_formatter/tests/specs/prettier/js/last-argument-expansion/break-parent.js.snap
@@ -35,21 +35,23 @@ true
 # Output
 ```js
 ({
-  processors: [
-    require(
-      "autoprefixer",
-      {
-        browsers: ["> 1%", "last 2 versions", "ie >= 11", "Firefox ESR"],
-      },
-    ),
-    require(
-      "postcss-url",
-    )({
-      url: (
-        url,
-      ) => url.startsWith("/") || /^[a-z]+:/.test(url) ? url : `/static/${url}`,
-    },),
-  ],
+  processors:
+    [
+      require(
+        "autoprefixer",
+        {
+          browsers: ["> 1%", "last 2 versions", "ie >= 11", "Firefox ESR"],
+        },
+      ),
+      require(
+        "postcss-url",
+      )({
+        url: (
+          url,
+        ) =>
+          url.startsWith("/") || /^[a-z]+:/.test(url) ? url : `/static/${url}`,
+      },),
+    ],
 });
 
 true ? test({

diff --git a/crates/rome_js_formatter/tests/specs/prettier/js/last-argument-expansion/edge_case.js.snap b/crates/rome_js_formatter/tests/specs/prettier/js/last-argument-expansion/edge_case.js.snap
@@ -78,21 +78,22 @@ a(
 exports.examples =
   [
     {
-      render: withGraphQLQuery(
-        "node(1234567890){image{uri}}",
-        function (container, data) {
-          return (
-            <div>
+      render:
+        withGraphQLQuery(
+          "node(1234567890){image{uri}}",
+          function (container, data) {
+            return (
+              <div>
             <InlineBlock>
               <img
                 src={data[1234567890].image.uri}
                 style={{position: 'absolute', top: '0', left: '0', zIndex:'-1'}}
               />
             </InlineBlock>
           </div>
-          );
-        },
-      ),
+            );
+          },
+        ),
     },
   ];
 
@@ -136,6 +137,6 @@ someReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReal
 ```
     2:   SomethingVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLong,
     5:       SomethingVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryVeryLong: 1,
-   31: someReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReally.a([
+   32: someReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReallyReally.a([
 ```