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
Conversation
For TypeScript, I think we can use |
Looks like there are some tests breaking related to flow upgrade. Maybe that should go in a separate PR? |
<Component /> | ||
<Component /> | ||
</ | ||
// close fragment |
There was a problem hiding this comment.
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}
/>;
There was a problem hiding this comment.
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>;
There was a problem hiding this comment.
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
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`fragment.js 1`] = ` | ||
<></>; |
There was a problem hiding this comment.
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
FYI Typescript 2.6.2 has been released with Fragments support (and React 16.2 has landed). |
This is now ready for review! |
tests/jsx_fragment/jsfmt.spec.js
Outdated
@@ -0,0 +1 @@ | |||
run_spec(__dirname); |
There was a problem hiding this comment.
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?
Also, failing test:
|
Uh, something went wrong with Travis. Gonna check what happened @azz I thought it ran for all 3 already.. sorry about that, will change it |
Yeah we should probably change that, it's a bit un-intuitive. #3355 |
Actually it was a bug with the 2nd print.. fixing |
src/printer.js
Outdated
n.body.type === "JSXElement" || | ||
n.body.type === "JSXFragment" || | ||
isJSXNode(n.body) || | ||
isJSXNode(n.body) || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh... thanks for catching
There was a problem hiding this comment.
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 :-)
This adds support for JSX Fragments (spec: facebook/jsx#93).
I only wrote simple tests for now, will probably update the already existing JSX tests to include fragments as soon as we can test with TypeScript. But regarding the core code this is ready for review.