From ad4ffb56b846baa324f17759ab652db856c85132 Mon Sep 17 00:00:00 2001 From: Karl O'Keeffe Date: Thu, 20 Apr 2017 09:22:38 +0100 Subject: [PATCH] Add new fill primitive and use it to wrap text in JSX This adds a new `fill` primitive that can be used to fill lines with as much code as possible before moving to a new line with the same indentation. It is used here layout JSX children. This gives us nicer wrapping for JSX elements containings lots of text interspersed with tags. --- src/doc-builders.js | 7 ++ src/doc-debug.js | 7 ++ src/doc-printer.js | 88 ++++++++++++++++++- src/doc-utils.js | 4 +- src/printer.js | 43 +++++---- .../__snapshots__/jsfmt.spec.js.snap | 3 +- .../__snapshots__/jsfmt.spec.js.snap | 34 +++---- .../__snapshots__/jsfmt.spec.js.snap | 4 +- .../__snapshots__/jsfmt.spec.js.snap | 3 +- .../__snapshots__/jsfmt.spec.js.snap | 71 +++++++++++++++ tests/jsx-text-wrap/jsfmt.spec.js | 1 + tests/jsx-text-wrap/test.js | 25 ++++++ 12 files changed, 243 insertions(+), 47 deletions(-) create mode 100644 tests/jsx-text-wrap/__snapshots__/jsfmt.spec.js.snap create mode 100644 tests/jsx-text-wrap/jsfmt.spec.js create mode 100644 tests/jsx-text-wrap/test.js diff --git a/src/doc-builders.js b/src/doc-builders.js index 0f1526be8175..820ccf49ea0b 100644 --- a/src/doc-builders.js +++ b/src/doc-builders.js @@ -57,6 +57,12 @@ function conditionalGroup(states, opts) { ); } +function fill(parts) { + parts.forEach(assertDoc); + + return { type: "fill", parts }; +} + function ifBreak(breakContents, flatContents) { if (breakContents) { assertDoc(breakContents); @@ -106,6 +112,7 @@ module.exports = { literalline, group, conditionalGroup, + fill, lineSuffix, lineSuffixBoundary, breakParent, diff --git a/src/doc-debug.js b/src/doc-debug.js index df3dd2794d8a..e0732d2a54d1 100644 --- a/src/doc-debug.js +++ b/src/doc-debug.js @@ -101,6 +101,13 @@ function printDoc(doc) { ); } + if (doc.type === "fill") { + return ("fill") + + "(" + + doc.parts.map(printDoc).join(", ") + + ")"; + } + if (doc.type === "line-suffix") { return "lineSuffix(" + printDoc(doc.contents) + ")"; } diff --git a/src/doc-printer.js b/src/doc-printer.js index 7e5acad251c7..7550bf5268d3 100644 --- a/src/doc-printer.js +++ b/src/doc-printer.js @@ -1,5 +1,12 @@ "use strict"; +const docBuilders = require("./doc-builders"); +const concat = docBuilders.concat; +const line = docBuilders.line; +const softline = docBuilders.softline; +const fill = docBuilders.fill; +const ifBreak = docBuilders.ifBreak; + const MODE_BREAK = 1; const MODE_FLAT = 2; @@ -30,7 +37,7 @@ function makeAlign(ind, n) { }; } -function fits(next, restCommands, width) { +function fits(next, restCommands, width, mustBeFlat) { let restIdx = restCommands.length; const cmds = [next]; while (width >= 0) { @@ -70,8 +77,17 @@ function fits(next, restCommands, width) { break; case "group": + if (mustBeFlat && doc.break) { + return false; + } cmds.push([ind, doc.break ? MODE_BREAK : mode, doc.contents]); + break; + case "fill": + for (var i = doc.parts.length - 1; i >= 0; i--) { + cmds.push([ind, mode, doc.parts[i]]); + } + break; case "if-break": if (mode === MODE_BREAK) { @@ -210,6 +226,76 @@ function printDocToString(doc, options) { break; } break; + + // Fills each line with as much code as possible before moving to a new + // line with the same indentation. + // + // Expects doc.parts to be an array of alternating code and + // whitespace. The whitespace contains the linebreaks. + // + // For example: + // ["I", line, "love", line, "monkeys"] + // or + // [{ type: group, ... }, softline, { type: group, ... }] + // + // It uses this parts structure to handle three main layout cases: + // * The first two non-whitespace items fit on the same line without + // breaking -> output both items and the whitespace "flat". + // * Only the first item fits on the line without breaking -> output the + // first item "flat" and the whitespace with "break". + // * Neither item fits on the line without breaking -> output the first + // item and the whitespace with "break". + case "fill": + let rem = width - pos; + + const parts = doc.parts; + if (parts.length === 0) { + break; + } + + const first = parts[0]; + const firstCmd = [ind, MODE_FLAT, first]; + if (parts.length === 1) { + if (fits(firstCmd, cmds, width - rem, true)) { + cmds.push(firstCmd); + } else { + cmds.push([ind, mode, first]); + } + break; + } + + const split = parts[1]; + if (parts.length === 2) { + if (fits(firstCmd, cmds, width - rem, true)) { + cmds.push([ind, MODE_FLAT, split]); + cmds.push(firstCmd); + } else { + cmds.push([ind, mode, split]); + cmds.push([ind, mode, first]); + } + break; + } + + const second = parts[2]; + const remaining = parts.slice(2); + const remainingCmd = [ind, MODE_BREAK, fill(remaining)]; + + const firstAndSecondCmd = [ind, MODE_FLAT, concat([first, split, second])]; + + if (fits(firstAndSecondCmd, cmds, rem, true)) { + cmds.push(remainingCmd) + cmds.push([ind, MODE_FLAT, split]); + cmds.push(firstCmd); + } else if (fits(firstCmd, cmds, width - rem, true)) { + cmds.push(remainingCmd) + cmds.push([ind, MODE_BREAK, split]); + cmds.push(firstCmd); + } else { + cmds.push(remainingCmd); + cmds.push([ind, MODE_BREAK, split]); + cmds.push([ind, MODE_BREAK, first]); + } + break; case "if-break": if (mode === MODE_BREAK) { if (doc.breakContents) { diff --git a/src/doc-utils.js b/src/doc-utils.js index 94b88ce9778f..656db6fab0ca 100644 --- a/src/doc-utils.js +++ b/src/doc-utils.js @@ -10,7 +10,7 @@ function traverseDoc(doc, onEnter, onExit, shouldTraverseConditionalGroups) { } if (shouldRecurse) { - if (doc.type === "concat") { + if (doc.type === "concat" || doc.type === "fill") { for (var i = 0; i < doc.parts.length; i++) { traverseDocRec(doc.parts[i]); } @@ -43,7 +43,7 @@ function traverseDoc(doc, onEnter, onExit, shouldTraverseConditionalGroups) { function mapDoc(doc, func) { doc = func(doc); - if (doc.type === "concat") { + if (doc.type === "concat" || doc.type === "fill") { return Object.assign({}, doc, { parts: doc.parts.map(d => mapDoc(d, func)) }); diff --git a/src/printer.js b/src/printer.js index ba5dfa5b9ade..64ee25bb2d69 100644 --- a/src/printer.js +++ b/src/printer.js @@ -17,6 +17,7 @@ var group = docBuilders.group; var indent = docBuilders.indent; var align = docBuilders.align; var conditionalGroup = docBuilders.conditionalGroup; +var fill = docBuilders.fill; var ifBreak = docBuilders.ifBreak; var breakParent = docBuilders.breakParent; var lineSuffixBoundary = docBuilders.lineSuffixBoundary; @@ -2915,8 +2916,8 @@ function printJSXChildren(path, options, print, jsxWhitespace) { if (/\S/.test(value)) { // treat each line of text as its own entity - value.split(/(\r?\n\s*)/).forEach(line => { - const newlines = line.match(/\n/g); + value.split(/(\r?\n\s*)/).forEach(textLine => { + const newlines = textLine.match(/\n/g); if (newlines) { children.push(hardline); @@ -2927,27 +2928,30 @@ function printJSXChildren(path, options, print, jsxWhitespace) { return; } - const beginSpace = /^\s+/.test(line); + const beginSpace = /^\s+/.test(textLine); if (beginSpace) { children.push(jsxWhitespace); - children.push(softline); } - const stripped = line.replace(/^\s+|\s+$/g, ""); + const stripped = textLine.replace(/^\s+|\s+$/g, ""); if (stripped) { - children.push(stripped); + + // Split text into words separated by "line"s. + stripped.split(/(\s+)/).forEach(word => { + const space = /\s+/.test(word); + if (space) { + children.push(line); + } else { + children.push(word); + } + }); } - const endSpace = /\s+$/.test(line); + const endSpace = /\s+$/.test(textLine); if (endSpace) { - children.push(softline); children.push(jsxWhitespace); } }); - - if (!isLineNext(util.getLast(children))) { - children.push(softline); - } } else if (/\n/.test(value)) { children.push(hardline); @@ -2959,14 +2963,15 @@ function printJSXChildren(path, options, print, jsxWhitespace) { // whitespace-only without newlines, // eg; a single space separating two elements children.push(jsxWhitespace); - children.push(softline); } } else { children.push(print(childPath)); - // add a line unless it's followed by a JSX newline + // add a softline where we have two adjacent JSX elements without + // any text or whitespace between them. let next = n.children[i + 1]; - if (!(next && /^\s*\n/.test(next.value))) { + const followedByJSXElement = next && next.type === 'JSXElement'; + if (followedByJSXElement) { children.push(softline); } } @@ -3024,8 +3029,8 @@ function printJSXElement(path, options, print) { let forcedBreak = willBreak(openingLines); const jsxWhitespace = options.singleQuote - ? ifBreak("{' '}", " ") - : ifBreak('{" "}', " "); + ? ifBreak(concat(["{' '}", softline]), " ") + : ifBreak(concat(['{" "}', softline]), " "); const children = printJSXChildren(path, options, print, jsxWhitespace); // Trim trailing lines, recording if there was a hardline @@ -3096,7 +3101,7 @@ function printJSXElement(path, options, print) { groups.map( contents => (Array.isArray(contents) - ? conditionalGroup([concat(contents)]) + ? conditionalGroup([fill(contents)]) : contents) ) ) @@ -3116,7 +3121,7 @@ function printJSXElement(path, options, print) { } return conditionalGroup([ - group(concat([openingLines, concat(children), closingLines])), + group(concat([openingLines, fill(children), closingLines])), multiLineElem ]); } diff --git a/tests/flow/more_react/__snapshots__/jsfmt.spec.js.snap b/tests/flow/more_react/__snapshots__/jsfmt.spec.js.snap index 6afb885f1877..dc7116992e32 100644 --- a/tests/flow/more_react/__snapshots__/jsfmt.spec.js.snap +++ b/tests/flow/more_react/__snapshots__/jsfmt.spec.js.snap @@ -164,7 +164,8 @@ var App = require("App.react"); var app = ( - {" "}// error, y: number but foo expects string in App.react + {" "} + // error, y: number but foo expects string in App.react Some text. ); diff --git a/tests/jsx-significant-space/__snapshots__/jsfmt.spec.js.snap b/tests/jsx-significant-space/__snapshots__/jsfmt.spec.js.snap index e1bb3b167212..7199c88099b7 100644 --- a/tests/jsx-significant-space/__snapshots__/jsfmt.spec.js.snap +++ b/tests/jsx-significant-space/__snapshots__/jsfmt.spec.js.snap @@ -90,8 +90,9 @@ before = ( before_break1 = ( - - {" "} + {" "} foo ); @@ -100,16 +101,14 @@ before_break2 = ( - {" "} + />{" "} foo ); after_break = ( - foo - {" "} + foo{" "} ); @@ -146,13 +145,15 @@ nest_plz = ( regression_not_transformed_1 = ( - {" "} + {" "} + ); regression_not_transformed_2 = ( {" "} + ); @@ -165,8 +166,8 @@ similar_1 = ( similar_2 = ( - - {" "} + {" "} + ); @@ -178,22 +179,15 @@ similar_3 = ( not_broken_end = (
- long text long text long text long text long text long text long text long text - {" "} - url - {" "} - long text long text + long text long text long text long text long text long text long text long + text url long text long text
); not_broken_begin = (
-
- {" "} - long text long text long text long text long text long text long text long text - url - {" "} - long text long text +
long text long text long text long text long text long text long text + long texturl long text long text
); diff --git a/tests/jsx-split-attrs/__snapshots__/jsfmt.spec.js.snap b/tests/jsx-split-attrs/__snapshots__/jsfmt.spec.js.snap index b672ecc84e8a..168dc233a19f 100644 --- a/tests/jsx-split-attrs/__snapshots__/jsfmt.spec.js.snap +++ b/tests/jsx-split-attrs/__snapshots__/jsfmt.spec.js.snap @@ -122,8 +122,7 @@ long_open_long_children = ( colour="blue" size="large" submitLabel="Sign in with Google" - /> - d + />d {" "} + ); diff --git a/tests/jsx-stateless-arrow-fn/__snapshots__/jsfmt.spec.js.snap b/tests/jsx-stateless-arrow-fn/__snapshots__/jsfmt.spec.js.snap index dd9d578781af..bd165a7e2bee 100644 --- a/tests/jsx-stateless-arrow-fn/__snapshots__/jsfmt.spec.js.snap +++ b/tests/jsx-stateless-arrow-fn/__snapshots__/jsfmt.spec.js.snap @@ -130,8 +130,7 @@ const render6 = ({ styles }) => ( attr4 > ddd d dd d d dddd dddd hello - - {" "} + {" "} hello diff --git a/tests/jsx-text-wrap/__snapshots__/jsfmt.spec.js.snap b/tests/jsx-text-wrap/__snapshots__/jsfmt.spec.js.snap new file mode 100644 index 000000000000..ff636bb529eb --- /dev/null +++ b/tests/jsx-text-wrap/__snapshots__/jsfmt.spec.js.snap @@ -0,0 +1,71 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`test.js 1`] = ` +// Wrapping text +x =
+ Some text that would need to wrap on to a new line in order to display correctly and nicely +
+ +// Wrapping tags +x =
+ f f f f f f +
+ +// Wrapping tags +x =
+ ffffff +
+ +// Wrapping tags +x = + +// Wrapping tags +x =
+ f +
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +// Wrapping text +x = ( +
+ Some text that would need to wrap on to a new line in order to display + correctly and nicely +
+); + +// Wrapping tags +x = ( +
+ f f f f{" "} + f f +
+); + +// Wrapping tags +x = ( +
+ ffff + ff +
+); + +// Wrapping tags +x = ( + +); + +// Wrapping tags +x = ( +
+ {" "} + f +
+); + +`; diff --git a/tests/jsx-text-wrap/jsfmt.spec.js b/tests/jsx-text-wrap/jsfmt.spec.js new file mode 100644 index 000000000000..7580dfab0b75 --- /dev/null +++ b/tests/jsx-text-wrap/jsfmt.spec.js @@ -0,0 +1 @@ +run_spec(__dirname, null, ["typescript"]); diff --git a/tests/jsx-text-wrap/test.js b/tests/jsx-text-wrap/test.js new file mode 100644 index 000000000000..ad519949aa1a --- /dev/null +++ b/tests/jsx-text-wrap/test.js @@ -0,0 +1,25 @@ +// Wrapping text +x =
+ Some text that would need to wrap on to a new line in order to display correctly and nicely +
+ +// Wrapping tags +x =
+ f f f f f f +
+ +// Wrapping tags +x =
+ ffffff +
+ +// Wrapping tags +x = + +// Wrapping tags +x =
+ f +