Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for JSX fragments #3237

Merged
merged 13 commits into from
Nov 30, 2017
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
},
"dependencies": {
"babel-code-frame": "7.0.0-alpha.12",
"babylon": "7.0.0-beta.28",
"babylon": "7.0.0-beta.31",
"camelcase": "4.1.0",
"chalk": "2.1.0",
"cjk-regex": "1.0.2",
Expand Down
81 changes: 49 additions & 32 deletions src/printer.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,12 +84,7 @@ function hasNodeIgnoreComment(node) {
function hasJsxIgnoreComment(path) {
const node = path.getValue();
const parent = path.getParentNode();
if (
!parent ||
!node ||
node.type !== "JSXElement" ||
parent.type !== "JSXElement"
) {
if (!parent || !node || !isJSXNode(node) || !isJSXNode(parent)) {
return false;
}

Expand Down Expand Up @@ -524,7 +519,8 @@ function genericPrintNoParens(path, options, print, args) {
(n.body.type === "ArrayExpression" ||
n.body.type === "ObjectExpression" ||
n.body.type === "BlockStatement" ||
n.body.type === "JSXElement" ||
isJSXNode(n.body) ||
isJSXNode(n.body) ||
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicated line?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh... thanks for catching

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I fixed before merging... lucky :-)

isTemplateOnItsOwnLine(n.body, options.originalText) ||
n.body.type === "ArrowFunctionExpression")
) {
Expand Down Expand Up @@ -1268,9 +1264,9 @@ function genericPrintNoParens(path, options, print, args) {
const lastConditionalParent = previousParent;

if (
n.test.type === "JSXElement" ||
n.consequent.type === "JSXElement" ||
n.alternate.type === "JSXElement" ||
isJSXNode(n.test) ||
isJSXNode(n.consequent) ||
isJSXNode(n.alternate) ||
conditionalExpressionChainContainsJSX(lastConditionalParent)
) {
jsxMode = true;
Expand Down Expand Up @@ -1741,7 +1737,7 @@ function genericPrintNoParens(path, options, print, args) {
n.expression.type === "JSXEmptyExpression" ||
n.expression.type === "TemplateLiteral" ||
n.expression.type === "TaggedTemplateExpression" ||
(parent.type === "JSXElement" &&
(isJSXNode(parent) &&
(n.expression.type === "ConditionalExpression" ||
isBinaryish(n.expression))));

Expand All @@ -1761,6 +1757,7 @@ function genericPrintNoParens(path, options, print, args) {
])
);
}
case "JSXFragment":
case "JSXElement": {
const elem = comments.printComments(
path,
Expand All @@ -1782,6 +1779,7 @@ function genericPrintNoParens(path, options, print, args) {

// don't break up opening elements with a single long text attribute
if (
n.attributes &&
n.attributes.length === 1 &&
n.attributes[0].value &&
isStringLiteral(n.attributes[0].value) &&
Expand Down Expand Up @@ -1841,6 +1839,22 @@ function genericPrintNoParens(path, options, print, args) {
}
case "JSXClosingElement":
return concat(["</", path.call(print, "name"), ">"]);
case "JSXOpeningFragment":
case "JSXClosingFragment": {
const hasOwnLineComment =
n.comments && !n.comments.every(util.isBlockComment);
return concat([
n.type === "JSXOpeningFragment" ? "<" : "</",
indent(
concat([
hasOwnLineComment ? hardline : "",
comments.printDanglingComments(path, options, true)
])
),
hasOwnLineComment ? hardline : "",
">"
]);
}
case "JSXText":
/* istanbul ignore next */
throw new Error("JSXTest should be handled by JSXElement");
Expand Down Expand Up @@ -2972,7 +2986,7 @@ function couldGroupArg(arg) {
arg.body.type === "ObjectExpression" ||
arg.body.type === "ArrayExpression" ||
arg.body.type === "CallExpression" ||
arg.body.type === "JSXElement"))
isJSXNode(arg.body)))
);
}

Expand Down Expand Up @@ -3958,6 +3972,10 @@ function printMemberChain(path, options, print) {
]);
}

function isJSXNode(node) {
return node.type === "JSXElement" || node.type === "JSXFragment";
}

function isEmptyJSXElement(node) {
if (node.children.length === 0) {
return true;
Expand Down Expand Up @@ -3991,9 +4009,7 @@ function isMeaningfulJSXText(node) {
}

function conditionalExpressionChainContainsJSX(node) {
return Boolean(
getConditionalChainContents(node).find(child => child.type === "JSXElement")
);
return Boolean(getConditionalChainContents(node).find(isJSXNode));
}

// If we have nested conditional expressions, we want to print them in JSX mode
Expand Down Expand Up @@ -4224,13 +4240,19 @@ function printJSXElement(path, options, print) {
const n = path.getValue();

// Turn <div></div> into <div />
if (isEmptyJSXElement(n)) {
if (n.type === "JSXElement" && isEmptyJSXElement(n)) {
n.openingElement.selfClosing = true;
delete n.closingElement;
return path.call(print, "openingElement");
}

const openingLines = path.call(print, "openingElement");
const closingLines = path.call(print, "closingElement");
const openingLines =
n.type === "JSXElement"
? path.call(print, "openingElement")
: path.call(print, "openingFragment");
const closingLines =
n.type === "JSXElement"
? path.call(print, "closingElement")
: path.call(print, "closingFragment");

if (
n.children.length === 1 &&
Expand All @@ -4245,12 +4267,6 @@ function printJSXElement(path, options, print) {
]);
}

// If no children, just print the opening element
if (n.openingElement.selfClosing) {
assert.ok(!n.closingElement);
return openingLines;
}

// Convert `{" "}` to text nodes containing a space.
// This makes it easy to turn them into `jsxWhitespace` which
// can then print as either a space or `{" "}` when breaking.
Expand All @@ -4265,12 +4281,12 @@ function printJSXElement(path, options, print) {
return child;
});

const containsTag =
n.children.filter(child => child.type === "JSXElement").length > 0;
const containsTag = n.children.filter(isJSXNode).length > 0;
const containsMultipleExpressions =
n.children.filter(child => child.type === "JSXExpressionContainer").length >
1;
const containsMultipleAttributes = n.openingElement.attributes.length > 1;
const containsMultipleAttributes =
n.type === "JSXElement" && n.openingElement.attributes.length > 1;

// Record any breaks. Should never go from true to false, only false to true.
let forcedBreak =
Expand Down Expand Up @@ -4408,6 +4424,7 @@ function maybeWrapJSXElementInParens(path, elem) {

const NO_WRAP_PARENTS = {
ArrayExpression: true,
JSXFragment: true,
JSXElement: true,
JSXExpressionContainer: true,
ExpressionStatement: true,
Expand Down Expand Up @@ -4458,7 +4475,7 @@ function shouldInlineLogicalExpression(node) {
return true;
}

if (node.right.type === "JSXElement") {
if (isJSXNode(node.right)) {
return true;
}

Expand Down Expand Up @@ -4642,7 +4659,7 @@ function hasTrailingComment(node) {
}

function hasLeadingOwnLineComment(text, node) {
if (node.type === "JSXElement") {
if (isJSXNode(node)) {
return hasNodeIgnoreComment(node);
}

Expand Down Expand Up @@ -4728,7 +4745,7 @@ function exprNeedsASIProtection(path, options) {
(node.operator === "+" || node.operator === "-")) ||
node.type === "TemplateLiteral" ||
node.type === "TemplateElement" ||
node.type === "JSXElement" ||
isJSXNode(node) ||
node.type === "BindExpression" ||
node.type === "RegExpLiteral" ||
(node.type === "Literal" && node.pattern) ||
Expand Down Expand Up @@ -5123,7 +5140,7 @@ function isTheOnlyJSXElementInMarkdown(options, path) {

const node = path.getNode();

if (!node.expression || node.expression.type !== "JSXElement") {
if (!node.expression || !isJSXNode(node.expression)) {
return false;
}

Expand Down
92 changes: 92 additions & 0 deletions tests/jsx_fragment/__snapshots__/jsfmt.spec.js.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`fragment.js 1`] = `
<></>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth adding a "complex" nested fragment example (here's the one from Babylon):

<div>
  <>
    <>
      <span>Hello</span>
      <span>world</span>
    </>
    <>
      <span>Goodbye</span>
      <span>world</span>
    </>
  </>
</div>

EDIT: nvm, noticed your comment re: tests in the PR desc


<>
text
</>;

<>
<Component />
<Component />
</>;

<>
text
<h2>heading</h2>
text
<h2>heading</h2>
text
</>;

<div>
<>
<>
<span>Hello</span>
<span>world</span>
</>
<>
<span>Goodbye</span>
<span>world</span>
</>
</>
</div>;

</* open fragment */>
<Component />
<Component />
</ /* close fragment */>;

< // open fragment
>
<Component />
<Component />
</ // close fragment
>;
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
<></>;

<>text</>;

<>
<Component />
<Component />
</>;

<>
text
<h2>heading</h2>
text
<h2>heading</h2>
text
</>;

<div>
<>
<>
<span>Hello</span>
<span>world</span>
</>
<>
<span>Goodbye</span>
<span>world</span>
</>
</>
</div>;

</* open fragment */>
<Component />
<Component />
<//* close fragment */>;

<
// open fragment
>
<Component />
<Component />
</
// close fragment
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For a self-closing JSX tag, we would put the "closing slash" after the comment or attributes. Maybe we should do the same thing for fragments?

Prettier 1.8.2
Playground link

Input:

<div
  /* foo */
/>;

<div
 a={a}
 b={reallyreallyreallyreallyreallyreallyreallyreallyreallyreallylong}
/>;

Output:

<div
/* foo */
/>;

<div
  a={a}
  b={reallyreallyreallyreallyreallyreallyreallyreallyreallyreallylong}
/>;

Copy link
Member Author

@duailibe duailibe Nov 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking more of this case:
Prettier 1.8.2
Playground link

Input:

<div>
  Text
</div /* comment */>;

<div>
  Text
</* comment */ /div>;

Output:

<div>Text</div /* comment */>;

<div>Text<//* comment */ div>;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you can't put attributes in a jsx fragment (afaik), I guess it only affects comments, so it's probably fine either way

>;

`;
43 changes: 43 additions & 0 deletions tests/jsx_fragment/fragment.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<></>;

<>
text
</>;

<>
<Component />
<Component />
</>;

<>
text
<h2>heading</h2>
text
<h2>heading</h2>
text
</>;

<div>
<>
<>
<span>Hello</span>
<span>world</span>
</>
<>
<span>Goodbye</span>
<span>world</span>
</>
</>
</div>;

</* open fragment */>
<Component />
<Component />
</ /* close fragment */>;

< // open fragment
>
<Component />
<Component />
</ // close fragment
>;
1 change: 1 addition & 0 deletions tests/jsx_fragment/jsfmt.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
run_spec(__dirname);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we turn on ["babylon", "typescript"], too?

6 changes: 3 additions & 3 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -783,9 +783,9 @@ babel-types@^6.26.0:
lodash "^4.17.4"
to-fast-properties "^1.0.3"

babylon@7.0.0-beta.28:
version "7.0.0-beta.28"
resolved "https://registry.yarnpkg.com/babylon/-/babylon-7.0.0-beta.28.tgz#14521f26a19918db2b5f4aca89e62e02e0644be7"
babylon@7.0.0-beta.31:
version "7.0.0-beta.31"
resolved "https://registry.yarnpkg.com/babylon/-/babylon-7.0.0-beta.31.tgz#7ec10f81e0e456fd0f855ad60fa30c2ac454283f"

babylon@^6.13.0, babylon@^6.17.2:
version "6.17.3"
Expand Down