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

Conversation

Projects
None yet
7 participants
@duailibe
Member

duailibe commented Nov 10, 2017

This adds support for JSX Fragments (spec: facebook/jsx#93).

  • Update babylon
  • Update flow (#3238)
  • Update TypeScript (#3331)
  • Write more comprehensive tests

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

This comment has been minimized.

Show comment
Hide comment
@ikatyang

ikatyang Nov 10, 2017

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@duailibe

duailibe Nov 10, 2017

Member

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

Member

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

This comment has been minimized.

@suchipi

suchipi Nov 10, 2017

Member

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}
/>;
@suchipi

suchipi Nov 10, 2017

Member

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}
/>;

This comment has been minimized.

@duailibe

duailibe Nov 10, 2017

Member

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>;
@duailibe

duailibe Nov 10, 2017

Member

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>;

This comment has been minimized.

@suchipi

suchipi Nov 10, 2017

Member

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

@suchipi

suchipi Nov 10, 2017

Member

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 referenced this pull request Nov 10, 2017

Merged

Update flow #3238

// Jest Snapshot v1, https://goo.gl/fbAQLP
exports[`fragment.js 1`] = `
<></>;

This comment has been minimized.

@existentialism

existentialism Nov 11, 2017

Collaborator

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

@existentialism

existentialism Nov 11, 2017

Collaborator

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 from Add support for JSX fragments to [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

This comment has been minimized.

Show comment
Hide comment
@beheh

beheh Nov 28, 2017

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

beheh commented Nov 28, 2017

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

@duailibe

This comment has been minimized.

Show comment
Hide comment
@duailibe

duailibe Nov 28, 2017

Member

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.

Member

duailibe commented Nov 28, 2017

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 from [WIP] Add support for JSX fragments to Add support for JSX fragments Nov 29, 2017

@duailibe

This comment has been minimized.

Show comment
Hide comment
@duailibe

duailibe Nov 29, 2017

Member

This is now ready for review!

Member

duailibe commented Nov 29, 2017

This is now ready for review!

duailibe added some commits Nov 29, 2017

@azz

azz approved these changes Nov 29, 2017

Show outdated Hide outdated tests/jsx_fragment/jsfmt.spec.js Outdated
@azz

This comment has been minimized.

Show comment
Hide comment
@azz

azz Nov 29, 2017

Member

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 />
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

This comment has been minimized.

Show comment
Hide comment
@duailibe

duailibe Nov 29, 2017

Member

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

Member

duailibe commented Nov 29, 2017

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

This comment has been minimized.

Show comment
Hide comment
@azz

azz Nov 29, 2017

Member

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

Member

azz commented Nov 29, 2017

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

duailibe added some commits Nov 30, 2017

@duailibe

This comment has been minimized.

Show comment
Hide comment
@duailibe

duailibe Nov 30, 2017

Member

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

Member

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 added some commits Nov 30, 2017

@duailibe duailibe merged commit b2cca7e into prettier:master Nov 30, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@duailibe duailibe deleted the duailibe:jsx-fragments branch Nov 30, 2017

Show outdated Hide outdated src/printer.js Outdated

@kyleshevlin kyleshevlin referenced this pull request Sep 5, 2018

Closed

Missing Support for JSX Fragment Syntax #136

9 of 9 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment