Prevent wrapping object properties with short keys (#10335)

* Prevent wrapping object properties with short keys * Update src/language-js/print/assignment.js Co-authored-by: Sosuke Suzuki <aosukeke@gmail.com> * Use getStringWidth instead of .length Co-authored-by: Sosuke Suzuki <aosukeke@gmail.com>
prettier · Mar 2, 2021 · 5f8fee4 · 5f8fee4
1 parent 2f384ab
commit 5f8fee4
Show file tree

Hide file tree

Showing 8 changed files with 140 additions and 27 deletions.
diff --git a/changelog_unreleased/javascript/10335.md b/changelog_unreleased/javascript/10335.md
@@ -0,0 +1,22 @@
+#### Prevent wrapping object properties with short keys (#10335 by @thorn0)
+
+Line breaks after short property names in object literals often look unnatural. Even when such a line break yields a line length benefit of 1 or 2 characters, it rarely looks justified. Prettier main avoids line breaks after property names shorter than `tabWidth + 3`. The "+3" part has been chosen rather arbitrarily. It may be revised upward in future versions, provided there is a really good rationale.
+
+<!-- prettier-ignore -->
+```js
+// Input
+const importantLink = {
+  url: "https://prettier.io/docs/en/rationale.html#what-prettier-is-concerned-about"
+};
+
+// Prettier stable
+const importantLink = {
+  url:
+    "https://prettier.io/docs/en/rationale.html#what-prettier-is-concerned-about"
+};
+
+// Prettier main
+const importantLink = {
+  url: "https://prettier.io/docs/en/rationale.html#what-prettier-is-concerned-about"
+};
+```
diff --git a/src/language-js/print/assignment.js b/src/language-js/print/assignment.js
@@ -1,8 +1,9 @@
 "use strict";
 
-const { isNonEmptyArray } = require("../../common/util");
+const { isNonEmptyArray, getStringWidth } = require("../../common/util");
 const {
   builders: { line, group, indent, indentIfBreak },
+  utils: { cleanDoc },
 } = require("../../document");
 const {
   hasLeadingOwnLineComment,
@@ -26,7 +27,7 @@ function printAssignment(
 ) {
   const rightDoc = path.call(print, rightPropertyName);
 
-  switch (chooseLayout(path, options, rightPropertyName)) {
+  switch (chooseLayout(path, options, leftDoc, rightPropertyName)) {
     // First break after operator, then the sides are broken independently on their own lines
     case "break-after-operator":
       return group([group(leftDoc), operator, group(indent([line, rightDoc]))]);
@@ -82,8 +83,9 @@ function printVariableDeclarator(path, options, print) {
   );
 }
 
-function chooseLayout(path, options, rightPropertyName) {
-  const rightNode = path.getValue()[rightPropertyName];
+function chooseLayout(path, options, leftDoc, rightPropertyName) {
+  const node = path.getValue();
+  const rightNode = node[rightPropertyName];
 
   if (!rightNode) {
     return "only-left";
@@ -119,18 +121,21 @@ function chooseLayout(path, options, rightPropertyName) {
     return "never-break-after-operator";
   }
 
-  if (shouldBreakAfterOperator(rightNode)) {
+  // wrapping object properties with very short keys usually doesn't add much value
+  const hasShortKey = isObjectPropertyWithShortKey(node, leftDoc, options);
+
+  if (shouldBreakAfterOperator(rightNode, hasShortKey)) {
     return "break-after-operator";
   }
 
-  if (shouldNeverBreakAfterOperator(rightNode)) {
+  if (hasShortKey || shouldNeverBreakAfterOperator(rightNode)) {
     return "never-break-after-operator";
   }
 
   return "fluid";
 }
 
-function shouldBreakAfterOperator(rightNode) {
+function shouldBreakAfterOperator(rightNode, hasShortKey) {
   if (isBinaryish(rightNode) && !shouldInlineLogicalExpression(rightNode)) {
     return true;
   }
@@ -147,6 +152,10 @@ function shouldBreakAfterOperator(rightNode) {
       return isNonEmptyArray(rightNode.decorators);
   }
 
+  if (hasShortKey) {
+    return false;
+  }
+
   let node = rightNode;
   for (;;) {
     if (node.type === "UnaryExpression") {
@@ -224,6 +233,26 @@ function isSimpleCall(node) {
   );
 }
 
+function isObjectPropertyWithShortKey(node, keyDoc, options) {
+  if (node.type !== "ObjectProperty" && node.type !== "Property") {
+    return false;
+  }
+  // TODO: for performance, it might make sense to use a more lightweight
+  // version of cleanDoc, such that it would stop once it detects that
+  // the doc can't be reduced to a string.
+  keyDoc = cleanDoc(keyDoc);
+  const MIN_OVERLAP_FOR_BREAK = 3;
+  //   ↓↓ - insufficient overlap for a line break
+  // key1: longValue1,
+  //   ↓↓↓↓↓↓ - overlap is long enough to break
+  // key2abcd:
+  //   longValue2
+  return (
+    typeof keyDoc === "string" &&
+    getStringWidth(keyDoc) < options.tabWidth + MIN_OVERLAP_FOR_BREAK
+  );
+}
+
 module.exports = {
   printVariableDeclarator,
   printAssignmentExpression,

diff --git a/tests/angular/angular/__snapshots__/jsfmt.spec.js.snap b/tests/angular/angular/__snapshots__/jsfmt.spec.js.snap
@@ -3558,18 +3558,16 @@ printWidth: 1
 <p>
   {{
     {
-      a:
-        !{}
+      a: !{}
     }
   }}
 </p>
 <p>
   {{
     {
-      a:
-        a
-          ? b
-          : {}
+      a: a
+        ? b
+        : {}
     }
   }}
 </p>

diff --git a/tests/js/object-prop-break-in/__snapshots__/jsfmt.spec.js.snap b/tests/js/object-prop-break-in/__snapshots__/jsfmt.spec.js.snap
@@ -36,8 +36,60 @@ const x = {
 
 =====================================output=====================================
 const x = {
-  ABC:
-    "12345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  ABC: "12345678901234567890123456789012345678901234567890123456789012345678901234567890",
+};
+
+================================================================================
+`;
+
+exports[`short-keys.js format 1`] = `
+====================================options=====================================
+parsers: ["babel", "flow", "typescript"]
+printWidth: 80
+                                                                                | printWidth
+=====================================input======================================
+var obj = {
+  // an entry with a very long string
+  x: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  url: 'http://example.com/12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890',
+  longName: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  [i]: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  [prop]: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  'x': "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  a: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  ab: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  abc: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  abcd: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  abcde: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  abcdef: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  '古': 'https://prettier.io/docs/en/rationale.html#what-prettier-is-concerned-about',
+  '古今': 'https://prettier.io/docs/en/rationale.html#what-prettier-is-concerned-about',
+  '古体诗': 'https://prettier.io/docs/en/rationale.html#what-prettier-is-concerned-about',
+};
+
+=====================================output=====================================
+var obj = {
+  // an entry with a very long string
+  x: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  url: "http://example.com/12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  longName:
+    "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  [i]: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  [prop]:
+    "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  x: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  a: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  ab: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  abc: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  abcd: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  abcde:
+    "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  abcdef:
+    "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  古: "https://prettier.io/docs/en/rationale.html#what-prettier-is-concerned-about",
+  古今: "https://prettier.io/docs/en/rationale.html#what-prettier-is-concerned-about",
+  古体诗:
+    "https://prettier.io/docs/en/rationale.html#what-prettier-is-concerned-about",
 };
 
 ================================================================================

diff --git a/tests/js/object-prop-break-in/short-keys.js b/tests/js/object-prop-break-in/short-keys.js
@@ -0,0 +1,18 @@
+var obj = {
+  // an entry with a very long string
+  x: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  url: 'http://example.com/12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890',
+  longName: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  [i]: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  [prop]: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  'x': "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  a: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  ab: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  abc: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  abcd: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  abcde: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  abcdef: "12345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890",
+  '古': 'https://prettier.io/docs/en/rationale.html#what-prettier-is-concerned-about',
+  '古今': 'https://prettier.io/docs/en/rationale.html#what-prettier-is-concerned-about',
+  '古体诗': 'https://prettier.io/docs/en/rationale.html#what-prettier-is-concerned-about',
+};
diff --git a/tests/js/ternaries/__snapshots__/jsfmt.spec.js.snap b/tests/js/ternaries/__snapshots__/jsfmt.spec.js.snap
@@ -892,10 +892,8 @@ a
 																),
 																a()
 																	? {
-																			a:
-																				a(),
-																			b:
-																				[],
+																			a: a(),
+																			b: [],
 																	  }
 																	: {},
 														  ]
@@ -910,8 +908,7 @@ a
 																	? [
 																			{
 																				a: 0,
-																				b:
-																					a(),
+																				b: a(),
 																			},
 																	  ]
 																	: a([

diff --git a/tests_integration/__tests__/__snapshots__/config-resolution.js.snap b/tests_integration/__tests__/__snapshots__/config-resolution.js.snap
@@ -47,8 +47,7 @@ function js() {
 
 module.exports =
         {
-                endOfLine:
-                        \\"auto\\",
+                endOfLine: \\"auto\\",
                 tabWidth: 8,
         };
 function noConfigJs() {

diff --git a/tests_integration/__tests__/__snapshots__/with-config-precedence.js.snap b/tests_integration/__tests__/__snapshots__/with-config-precedence.js.snap
@@ -126,8 +126,7 @@ function js() {
 
 module.exports =
         {
-                endOfLine:
-                        \\"auto\\",
+                endOfLine: \\"auto\\",
                 tabWidth: 8,
         };
 function noConfigJs() {
@@ -222,8 +221,7 @@ function js() {
 
 module.exports =
         {
-                endOfLine:
-                        \\"auto\\",
+                endOfLine: \\"auto\\",
                 tabWidth: 8,
         };
 function noConfigJs() {