From a680cd8534e6df3f1982efc093a5b8deccee9801 Mon Sep 17 00:00:00 2001
From: Karl O'Keeffe <github@monket.net>
Date: Wed, 31 May 2017 17:42:36 +0100
Subject: [PATCH] Fix unstable JSX output by ensuring we follow `fill` rules.
 (#1827)

* Fix unstable JSX output by ensuring we follow `fill` rules.

This changes makes us more strict about ensuring our JSX children follow the alternating content/whitespace format expected by the `fill` primitive.

Previously there were some cases where could get out of sync which would throw out the formatting.

* Simplify whitespace wrangling

# Conflicts:
#	src/printer.js
---
 src/printer.js                                | 102 +++++++++++-------
 .../__snapshots__/jsfmt.spec.js.snap          |  49 +++++++++
 tests/jsx-text-wrap/test.js                   |  18 ++++
 3 files changed, 128 insertions(+), 41 deletions(-)

diff --git a/src/printer.js b/src/printer.js
index 71214faa..ab8ca2f6 100644
--- a/src/printer.js
+++ b/src/printer.js
@@ -3861,6 +3861,11 @@ function isEmptyJSXElement(node) {
 //
 // For another, leading, trailing, and lone whitespace all need to
 // turn themselves into the rather ugly `{' '}` when breaking.
+//
+// Finally we print JSX using the `fill` doc primitive.
+// This requires that we give it an array of alternating
+// content and whitespace elements.
+// To ensure this we add dummy `""` content elements as needed.
 function printJSXChildren(path, options, print, jsxWhitespace) {
   const n = path.getValue();
   const children = [];
@@ -3877,10 +3882,12 @@ function printJSXChildren(path, options, print, jsxWhitespace) {
         value.split(/(\r?\n\s*)/).forEach(textLine => {
           const newlines = textLine.match(/\n/g);
           if (newlines) {
+            children.push("");
             children.push(hardline);
 
             // allow one extra newline
             if (newlines.length > 1) {
+              children.push("");
               children.push(hardline);
             }
             return;
@@ -3892,6 +3899,23 @@ function printJSXChildren(path, options, print, jsxWhitespace) {
 
           const beginSpace = /^[ \n\r\t]+/.test(textLine);
           if (beginSpace) {
+            children.push("");
+            children.push(jsxWhitespace);
+          }
+
+          const stripped = textLine.replace(/^[ \n\r\t]+|[ \n\r\t]+$/g, "");
+          // Split text into words separated by "line"s.
+          stripped.split(/([ \n\r\t]+)/).forEach(word => {
+            const space = /[ \n\r\t]+/.test(word);
+            if (space) {
+              children.push(line);
+            } else {
+              children.push(word);
+            }
+          });
+
+          const endSpace = /[ \n\r\t]+$/.test(textLine);
+          if (endSpace) {
             children.push(jsxWhitespace);
           } else {
             // Ideally this would be a `softline` to allow a break between
@@ -3905,57 +3929,39 @@ function printJSXChildren(path, options, print, jsxWhitespace) {
             // adverse effect on formatting algorithm.
             children.push("");
           }
-
-          const stripped = textLine.replace(/^[ \n\r\t]+|[ \n\r\t]+$/g, "");
-          if (stripped) {
-            // Split text into words separated by "line"s.
-            stripped.split(/([ \n\r\t]+)/).forEach(word => {
-              const space = /[ \n\r\t]+/.test(word);
-              if (space) {
-                children.push(line);
-              } else {
-                children.push(word);
-              }
-            });
-          }
-
-          const endSpace = /[ \n\r\t]+$/.test(textLine);
-          if (endSpace) {
-            children.push(jsxWhitespace);
-          } else {
-            // As above this would ideally be a `softline`.
-            children.push("");
-          }
         });
       } else if (/\n/.test(value)) {
+        children.push("");
         children.push(hardline);
 
         // allow one extra newline
         if (value.match(/\n/g).length > 1) {
+          children.push("");
           children.push(hardline);
         }
       } else if (/[ \n\r\t]/.test(value)) {
         // whitespace(s)-only without newlines,
         // eg; one or more spaces separating two elements
         for (let i = 0; i < value.length; ++i) {
-          children.push(jsxWhitespace);
           // Because fill expects alternating content and whitespace parts
-          // we need to include an empty content part between each JSX
+          // we need to include an empty content part before each JSX
           // whitespace.
-          if (i + 1 < value.length) {
-            children.push("");
-          }
+          children.push("");
+          children.push(jsxWhitespace);
         }
       }
     } else {
       children.push(print(childPath));
 
-      // add a softline where we have two adjacent elements that are not
-      // literals
       const next = n.children[i + 1];
       const followedByJSXElement = next && !isLiteral(next);
       if (followedByJSXElement) {
         children.push(softline);
+      } else {
+        // Ideally this would be a softline as well.
+        // See the comment above about the Facebook translation pipeline as
+        // to why this is an empty string.
+        children.push("");
       }
     }
   }, "children");
@@ -4019,9 +4025,20 @@ function printJSXElement(path, options, print) {
 
   const children = printJSXChildren(path, options, print, jsxWhitespace);
 
-  // Trim trailing lines, recording if there was a hardline
+  // Remove multiple filler empty strings
+  // These can occur when a text element is followed by a newline.
+  for (let i = children.length - 2; i >= 0; i--) {
+    if (children[i] === "" && children[i + 1] === "") {
+      children.splice(i, 2);
+    }
+  }
+
+  // Trim trailing lines (or empty strings), recording if there was a hardline
   let numTrailingHard = 0;
-  while (children.length && isLineNext(util.getLast(children))) {
+  while (
+    children.length &&
+    (isLineNext(util.getLast(children)) || isEmpty(util.getLast(children)))
+  ) {
     if (willBreak(util.getLast(children))) {
       ++numTrailingHard;
       forcedBreak = true;
@@ -4030,21 +4047,28 @@ function printJSXElement(path, options, print) {
   }
   // allow one extra newline
   if (numTrailingHard > 1) {
+    children.push("");
     children.push(hardline);
   }
 
   // Trim leading lines (or empty strings), recording if there was a hardline
   let numLeadingHard = 0;
-  while (children.length && (isLineNext(children[0]) || isEmpty(children[0]))) {
-    if (willBreak(children[0])) {
+  while (
+    children.length &&
+    (isLineNext(children[0]) || isEmpty(children[0])) &&
+    (isLineNext(children[1]) || isEmpty(children[1]))
+  ) {
+    if (willBreak(children[0]) || willBreak(children[1])) {
       ++numLeadingHard;
       forcedBreak = true;
     }
     children.shift();
+    children.shift();
   }
   // allow one extra newline
   if (numLeadingHard > 1) {
     children.unshift(hardline);
+    children.unshift("");
   }
 
   // Tweak how we format children if outputting this element over multiple lines.
@@ -4054,15 +4078,11 @@ function printJSXElement(path, options, print) {
     // Ensure that we display leading, trailing, and solitary whitespace as
     // `{" "}` when outputting this element over multiple lines.
     if (child === jsxWhitespace) {
-      if (children.length === 1) {
-        multilineChildren.push(rawJsxWhitespace);
-        return;
-      } else if (i === 0) {
-        // Fill expects alternating content & whitespace parts
-        // always starting with content.
-        // So we add a dummy content element if we would otherwise start
-        // with whitespace.
-        multilineChildren.push("");
+      if (i === 1 && children[i - 1] === "") {
+        if (children.length === 2) {
+          multilineChildren.push(rawJsxWhitespace);
+          return;
+        }
         multilineChildren.push(concat([rawJsxWhitespace, hardline]));
         return;
       } else if (i === children.length - 1) {
diff --git a/tests/jsx-text-wrap/__snapshots__/jsfmt.spec.js.snap b/tests/jsx-text-wrap/__snapshots__/jsfmt.spec.js.snap
index 1dde8e08..e3260f5c 100644
--- a/tests/jsx-text-wrap/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/jsx-text-wrap/__snapshots__/jsfmt.spec.js.snap
@@ -117,6 +117,24 @@ this_really_should_split_across_lines =
   <div>
     before{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after
   </div>
+
+
+unstable_before =
+  <div className="yourScore">
+    Your score: <span className="score">{\`\${mini.crosstable.users[sessionUserId]} - \${mini.crosstable.users[user.id]}\`}</span>
+  </div>
+
+unstable_after_first_run = (
+  <div className="yourScore">
+    Your score:
+    {" "}
+    <span className="score">{\`\${mini.crosstable.users[sessionUserId]} - \${mini
+      .crosstable.users[user.id]}\`}</span>
+  </div>
+);
+
+solitary_whitespace =
+  <div first="first" second="second" third="third" fourth="fourth" fifth="fifth" sixth="sixth"> </div>
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 // Wrapping text
 x = (
@@ -293,4 +311,35 @@ this_really_should_split_across_lines = (
   </div>
 );
 
+unstable_before = (
+  <div className="yourScore">
+    Your score:
+    {" "}
+    <span className="score">{\`\${mini.crosstable.users[sessionUserId]} - \${mini
+      .crosstable.users[user.id]}\`}</span>
+  </div>
+);
+
+unstable_after_first_run = (
+  <div className="yourScore">
+    Your score:
+    {" "}
+    <span className="score">{\`\${mini.crosstable.users[sessionUserId]} - \${mini
+      .crosstable.users[user.id]}\`}</span>
+  </div>
+);
+
+solitary_whitespace = (
+  <div
+    first="first"
+    second="second"
+    third="third"
+    fourth="fourth"
+    fifth="fifth"
+    sixth="sixth"
+  >
+    {" "}
+  </div>
+);
+
 `;
diff --git a/tests/jsx-text-wrap/test.js b/tests/jsx-text-wrap/test.js
index 6ce67e97..74405442 100644
--- a/tests/jsx-text-wrap/test.js
+++ b/tests/jsx-text-wrap/test.js
@@ -114,3 +114,21 @@ this_really_should_split_across_lines =
   <div>
     before{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after{stuff}after
   </div>
+
+
+unstable_before =
+  <div className="yourScore">
+    Your score: <span className="score">{`${mini.crosstable.users[sessionUserId]} - ${mini.crosstable.users[user.id]}`}</span>
+  </div>
+
+unstable_after_first_run = (
+  <div className="yourScore">
+    Your score:
+    {" "}
+    <span className="score">{`${mini.crosstable.users[sessionUserId]} - ${mini
+      .crosstable.users[user.id]}`}</span>
+  </div>
+);
+
+solitary_whitespace =
+  <div first="first" second="second" third="third" fourth="fourth" fifth="fifth" sixth="sixth"> </div>