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
Merged

Conversation

duailibe
Copy link
Member

@duailibe duailibe commented Nov 10, 2017

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.

@ikatyang
Copy link
Member

ikatyang commented Nov 10, 2017

For TypeScript, I think we can use typescript@next directly, which is the daily build from TS master, but I guess we'll have to get updates from typescript-eslint-parser first.

@duailibe
Copy link
Member Author

duailibe commented Nov 10, 2017

Looks like there are some tests breaking related to flow upgrade. Maybe that should go in a separate PR?

<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

@duailibe duailibe mentioned this pull request Nov 10, 2017
// 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

@duailibe duailibe changed the title Add support for JSX fragments [WIP] Add support for JSX fragments Nov 17, 2017
This was referenced Nov 27, 2017
@duailibe duailibe added this to the 1.9 milestone Nov 28, 2017
@beheh
Copy link

beheh commented Nov 28, 2017

FYI Typescript 2.6.2 has been released with Fragments support (and React 16.2 has landed).

@duailibe
Copy link
Member Author

Thanks @beheh. The typescript upgrade is already in progress, see #3331.

We were on 2.5 and there were some changes so there are a few things we need to work on.

@duailibe duailibe changed the title [WIP] Add support for JSX fragments Add support for JSX fragments Nov 29, 2017
@duailibe
Copy link
Member Author

This is now ready for review!

@@ -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?

@azz
Copy link
Member

azz commented Nov 29, 2017

Also, failing test:

 FAIL  tests/jsx_fragment/jsfmt.spec.js

  ● /home/travis/build/prettier/prettier/tests/jsx_fragment/fragment.js parse

    expect(received).toBe(expected)
    
    Expected value to be (using ===):

      null

    Received:

      "SyntaxError: Unexpected token < (36:1)

      34 | <//* close fragment */>;
      35 | 
    > 36 | <
         | ^
      37 |   // open fragment
      38 | >
      39 |   <Component />

@duailibe
Copy link
Member Author

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

@azz
Copy link
Member

azz commented Nov 29, 2017

Yeah we should probably change that, it's a bit un-intuitive. #3355

@duailibe
Copy link
Member Author

duailibe commented Nov 30, 2017

Okay... looks like </ /* comment */>;

doesn't work correctly with travis... it treats / /* as // so the whole line is a comment. This test passes locally thought, not sure what's up.

I'll remove that test..

Actually it was a bug with the 2nd print.. fixing

@duailibe duailibe merged commit b2cca7e into prettier:master Nov 30, 2017
@duailibe duailibe deleted the jsx-fragments branch November 30, 2017 03:09
src/printer.js Outdated
n.body.type === "JSXElement" ||
n.body.type === "JSXFragment" ||
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 :-)

@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jan 19, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants