Repository: jlongster/prettier
Pull request #496 (2017-01-27 18:30:15)

Simplify arrow functions that use blocks
Alright, I had pushed a different branch and written up a long PR comment about tweaking the low-level printer's remeasuring logic. However, it made the low-level printer even more complicated, and I'm not willing to do that after I found this really simple fix.

The gist of it is that the printer has problems with a specific combination of groups. It needs to "remeasure" the code when it hits a hard line in flat mode; the printer thinks the whole expression fits because it hit a newline, but since it was a hard line, only the first line is sure to fit. The rest of the contents need to be "remeasured", so when a line is outputted we toggle a flag to force the printer to do that.

Unfortunately, there seems to be an edge case where a global flag doesn't work. If there are multiple groups that exist in just the right place, they will all inherit "flat" mode, but only the first one will be remeasured. That's why this was fixed by putting other `group`s in arbitrary places because it changed the document structure which changed the flags it inherited.

The re-measurement logic was written before we propagated breaks all the way up. After thinking through this for a while, I'm not sure we even need it anymore, because any hard lines are going to force all groups to have `shouldBreak: true` which will force a remeasurement anyway. However, it looks like I can't quite remove it yet because there are still some rare cases with `conditionalGroup` that desire to output a hard line in "flat" mode. There are 2 snapshot tests that change when I remove remeasuring.

I hope in the future I can remove remeasuring though, and I'd like to leave as it instead of complicate it more, and implement this simple fix.

This is the right fix because we do not need the complicated `conditionalGroup` if the arrow function is using a block anyway. By avoiding cG, we avoid this bug, and the arrow function is the only case I can find where it's trying to do this weird behavior.


Patch:
diff --git a/src/printer.js b/src/printer.js
index 09e463da9..454aaae3d 100644
--- a/src/printer.js
+++ b/src/printer.js
@@ -284,7 +284,8 @@ function genericPrintNoParens(path, options, print) {
       if (
         n.body.type === "ArrayExpression" ||
           n.body.type === "ObjectExpression" ||
-          n.body.type === "JSXElement"
+          n.body.type === "JSXElement" ||
+          n.body.type === "BlockStatement"
       ) {
         return group(collapsed);
       }


Test patch:
diff --git a/tests/arrows/__snapshots__/jsfmt.spec.js.snap b/tests/arrows/__snapshots__/jsfmt.spec.js.snap
index b5a444692..2e626000b 100644
--- a/tests/arrows/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/arrows/__snapshots__/jsfmt.spec.js.snap
@@ -34,3 +34,19 @@ veryLongCall(
 );
 "
 `;
+
+exports[`test long-contents.js 1`] = `
+"const foo = () => {
+  expect(arg1, arg2, arg3).toEqual({message: \'test\', messageType: \'SMS\', status: \'Unknown\', created: \'11/01/2017 13:36\'});
+};
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+const foo = () => {
+  expect(arg1, arg2, arg3).toEqual({
+    message: \"test\",
+    messageType: \"SMS\",
+    status: \"Unknown\",
+    created: \"11/01/2017 13:36\"
+  });
+};
+"
+`;
diff --git a/tests/arrows/long-contents.js b/tests/arrows/long-contents.js
new file mode 100644
index 000000000..e4d78f580
--- /dev/null
+++ b/tests/arrows/long-contents.js
@@ -0,0 +1,3 @@
+const foo = () => {
+  expect(arg1, arg2, arg3).toEqual({message: 'test', messageType: 'SMS', status: 'Unknown', created: '11/01/2017 13:36'});
+};