Skip to content

Commit

Permalink
New primitive to fill a line with as many doc parts as possible (#1120)
Browse files Browse the repository at this point in the history 
* 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.

* Quick fix for jsx whitespace regressions

* Fix a couple more bugs

* Tidy up the `fill` algorithm

Attempt to make the algorithm a little more regular, and improve the naming of variables to make it a little easier to understand (I hope!).

* Small tidy up of JSX whitespace declarations

* Remove unnecessary code

It turns out that `children` is only used in the case when the element is printed on a single line, in which case all the types of JSX whitespaces behave the same, so we don't need to special case leading/trailing/solitary whitespace.

* A little more tidy up based on PR feedback

* Fix up tests after rebasing

* Make it explicit that we keep multiple consecutive spaces

* Add an explanatory comment

* Fix broken snapshot in master

* Ignore existing commands when deciding whether content will fit when using fill

* Fix a bug where children would get incorrectly filled onto a line

* Tidy up JSX whitespace names
  • Loading branch information
@karl @vjeux
karl authored and vjeux committed May 10, 2017
1 parent d4217f5 commit 5cda955
Show file tree
Hide file tree
Showing 12 changed files with 521 additions and 70 deletions.
7 changes: 7 additions & 0 deletions src/doc-builders.js
View file
Expand Up @@ -54,6 +54,12 @@ function conditionalGroup(states, opts) {
);
}

function fill(parts) {
parts.forEach(assertDoc);

return { type: "fill", parts };
}

function ifBreak(breakContents, flatContents) {
if (breakContents) {
assertDoc(breakContents);
Expand Down Expand Up @@ -103,6 +109,7 @@ module.exports = {
literalline,
group,
conditionalGroup,
fill,
lineSuffix,
lineSuffixBoundary,
breakParent,
Expand Down
7 changes: 7 additions & 0 deletions src/doc-debug.js
View file
Expand Up @@ -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) + ")";
}
Expand Down
98 changes: 97 additions & 1 deletion src/doc-printer.js
View file
@@ -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;

Expand Down Expand Up @@ -40,7 +47,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) {
Expand Down Expand Up @@ -80,8 +87,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) {
Expand Down Expand Up @@ -220,6 +236,86 @@ 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 content 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 content items fit on the same line without
// breaking
// -> output the first content item and the whitespace "flat".
// * Only the first content item fits on the line without breaking
// -> output the first content item "flat" and the whitespace with
// "break".
// * Neither content item fits on the line without breaking
// -> output the first content item and the whitespace with "break".
case "fill": {
let rem = width - pos;

const parts = doc.parts;
if (parts.length === 0) {
break;
}

const content = parts[0];
const contentFlatCmd = [ind, MODE_FLAT, content];
const contentBreakCmd = [ind, MODE_BREAK, content];
const contentFits = fits(contentFlatCmd, [], width - rem, true)

if (parts.length === 1) {
if (contentFits) {
cmds.push(contentFlatCmd);
} else {
cmds.push(contentBreakCmd);
}
break;
}

const whitespace = parts[1];
const whitespaceFlatCmd = [ind, MODE_FLAT, whitespace]
const whitespaceBreakCmd = [ind, MODE_BREAK, whitespace]

if (parts.length === 2) {
if (contentFits) {
cmds.push(whitespaceFlatCmd);
cmds.push(contentFlatCmd);
} else {
cmds.push(whitespaceBreakCmd);
cmds.push(contentBreakCmd);
}
break;
}

const remaining = parts.slice(2);
const remainingCmd = [ind, mode, fill(remaining)];

const secondContent = parts[2];
const firstAndSecondContentFlatCmd = [ind, MODE_FLAT, concat([content, whitespace, secondContent])];
const firstAndSecondContentFits = fits(firstAndSecondContentFlatCmd, [], rem, true);

if (firstAndSecondContentFits) {
cmds.push(remainingCmd)
cmds.push(whitespaceFlatCmd);
cmds.push(contentFlatCmd);
} else if (contentFits) {
cmds.push(remainingCmd)
cmds.push(whitespaceBreakCmd);
cmds.push(contentFlatCmd);
} else {
cmds.push(remainingCmd);
cmds.push(whitespaceBreakCmd);
cmds.push(contentBreakCmd);
}
break;
}
case "if-break":
if (mode === MODE_BREAK) {
if (doc.breakContents) {
Expand Down
4 changes: 2 additions & 2 deletions src/doc-utils.js
View file
Expand Up @@ -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]);
}
Expand Down Expand Up @@ -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))
});
Expand Down
106 changes: 53 additions & 53 deletions src/printer.js
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -3455,8 +3456,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);

Expand All @@ -3467,27 +3468,38 @@ function printJSXChildren(path, options, print, jsxWhitespace) {
return;
}

const beginSpace = /^\s+/.test(line);
if (textLine.length === 0) {
return;
}

const beginSpace = /^\s+/.test(textLine);
if (beginSpace) {
children.push(jsxWhitespace);
} else {
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);
} else {
children.push(softline);
}
});

if (!isLineNext(util.getLast(children))) {
children.push(softline);
}
} else if (/\n/.test(value)) {
children.push(hardline);

Expand All @@ -3500,15 +3512,22 @@ function printJSXChildren(path, options, print, jsxWhitespace) {
// 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
// whitespace.
if (i + 1 < value.length) {
children.push('');
}
}
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 elements that are not
// literals
let next = n.children[i + 1];
if (!(next && /^\s*\n/.test(next.value))) {
const followedByJSXElement = next && !namedTypes.Literal.check(next);
if (followedByJSXElement) {
children.push(softline);
}
}
Expand Down Expand Up @@ -3565,9 +3584,9 @@ function printJSXElement(path, options, print) {
// Record any breaks. Should never go from true to false, only false to true.
let forcedBreak = willBreak(openingLines);

const jsxWhitespace = options.singleQuote
? ifBreak("{' '}", " ")
: ifBreak('{" "}', " ");
const rawJsxWhitespace = options.singleQuote ? "{' '}" : '{" "}';
const jsxWhitespace = ifBreak(concat([softline, rawJsxWhitespace, softline]), " ")

const children = printJSXChildren(path, options, print, jsxWhitespace);

// Trim trailing lines, recording if there was a hardline
Expand Down Expand Up @@ -3598,56 +3617,37 @@ function printJSXElement(path, options, print) {
children.unshift(hardline);
}

// Group by line, recording if there was a hardline.
let groups = [[]]; // Initialize the first line's group
// Tweak how we format children if outputting this element over multiple lines.
// Also detect whether we will force this element to output over multiple lines.
let multilineChildren = [];
children.forEach((child, i) => {
// leading and trailing JSX whitespace don't go into a group

// Ensure that we display leading, trailing, and solitary whitespace as
// `{" "}` when outputting this element over multiple lines.
if (child === jsxWhitespace) {
if (i === 0) {
groups.unshift(child);
if (children.length === 1) {
multilineChildren.push(rawJsxWhitespace);
return;
} else if (i === 0) {
multilineChildren.push(concat([rawJsxWhitespace, hardline]));
return;
} else if (i === children.length - 1) {
groups.push(child);
multilineChildren.push(concat([hardline, rawJsxWhitespace]));
return;
}
}

let prev = children[i - 1];
if (prev && willBreak(prev)) {
forcedBreak = true;
multilineChildren.push(child);

// On a new line, so create a new group and put this element in it.
groups.push([child]);
} else {
// Not on a newline, so add this element to the current group.
util.getLast(groups).push(child);
}

// Ensure we record hardline of last element.
if (!forcedBreak && i === children.length - 1) {
if (willBreak(child)) forcedBreak = true;
if (willBreak(child)) {
forcedBreak = true;
}
});

const childrenGroupedByLine = [
hardline,
// Conditional groups suppress break propagation; we want to output
// hard lines without breaking up the entire jsx element.
// Note that leading and trailing JSX Whitespace don't go into a group.
concat(
groups.map(
contents =>
(Array.isArray(contents)
? conditionalGroup([concat(contents)])
: contents)
)
)
];

const multiLineElem = group(
concat([
openingLines,
indent(group(concat(childrenGroupedByLine), { shouldBreak: true })),
indent(concat([hardline, fill(multilineChildren)])),
hardline,
closingLines
])
Expand All @@ -3658,7 +3658,7 @@ function printJSXElement(path, options, print) {
}

return conditionalGroup([
group(concat([openingLines, concat(children), closingLines])),
group(concat([openingLines, fill(children), closingLines])),
multiLineElem
]);
}
Expand Down
3 changes: 2 additions & 1 deletion tests/flow/more_react/__snapshots__/jsfmt.spec.js.snap
View file
Expand Up @@ -164,7 +164,8 @@ var App = require("App.react");
var app = (
<App y={42}>
{" "}// error, y: number but foo expects string in App.react
{" "}
// error, y: number but foo expects string in App.react
Some text.
</App>
);
Expand Down

0 comments on commit 5cda955

Please sign in to comment.