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
2017-05-31 17:42:36 +01:00 · 2017-05-31 17:42:36 +01:00 · a680cd8534
parent fa708d102a
commit a680cd8534
3 changed files with 128 additions and 41 deletions
--- 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) {
--- 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>
+);
+
 `;
--- 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>