Skip to content
This repository has been archived by the owner on May 19, 2018. It is now read-only.

Add JSX fragment syntax support #755

Merged
merged 3 commits into from
Oct 28, 2017
Merged

Conversation

clemmy
Copy link
Contributor

@clemmy clemmy commented Oct 11, 2017

Q A
Bug fix? no
Breaking change? no
New feature? yes
Deprecations? no
Spec compliancy? yes
Tests added/pass? yes
Fixed tickets
License MIT

This PR adds support for facebook/jsx#93.

I'm also currently working on corresponding Babel PR as well, just leaving this up here first for review, since I'm not sure if I'm abusing $FlowIgnore. 😆

UPDATE: Both PR's ready for review and Babel one can be found at babel/babel#6552

@hzoo hzoo requested a review from sebmarkbage October 11, 2017 01:29
Copy link
Member

@Jessidhia Jessidhia left a comment

Choose a reason for hiding this comment

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

Will you update the parser to include facebook/jsx#94 here, or later after it is merged?

Also would be nice to have a test case with a JSXExpressionContainer directly inside a JSXFragment. <><div>JSXElement</div>JSXText{'JSXExpressionContainer'}</>

| ?N.JSXOpeningElement
| ?N.JSXClosingElement
| ?N.JSXOpeningFragment
| ?N.JSXClosingFragment,
Copy link
Member

Choose a reason for hiding this comment

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

Fragments cannot be self-closing, but it might be good to just accept any N.JSXElement to make it easier to test against arbitrary elements.

Although this is an internal use only function, so maybe it doesn't need to be extra ergonomic 🤔

| ?N.JSXClosingElement
| ?N.JSXOpeningFragment
| ?N.JSXClosingFragment,
): boolean {
Copy link
Member

Choose a reason for hiding this comment

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

Does Flow support type narrowing similarly to TypeScript? If it does, the return type could be something similar to : object is N.JSXOpeningFragment | N.JSXClosingFragment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not that I can find in the docs! :(

@@ -410,8 +431,18 @@ export default (superClass: Class<Parser>): Class<Parser> =>

if (
// $FlowIgnore
Copy link
Member

Choose a reason for hiding this comment

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

All these $FlowIgnore are scary...... but they were already in place 🤷‍♀️

@@ -799,6 +799,9 @@ export type JSXAttribute = Node;
export type JSXOpeningElement = Node;
export type JSXClosingElement = Node;
export type JSXElement = Node;
export type JSXOpeningFragment = Node;
export type JSXClosingFragment = Node;
export type JSXFragment = Node;
Copy link
Member

Choose a reason for hiding this comment

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

Ah, that's why there are so many $FlowIgnore in the JSX parser. None of these types are written 😆

(Even if you feel inclined to, don't do it on this PR, of course)

@@ -0,0 +1 @@
<>Hi, I'm a string!</>
Copy link
Member

Choose a reason for hiding this comment

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

This will be very confusing for syntax highlighters for a while...

@clemmy
Copy link
Contributor Author

clemmy commented Oct 12, 2017

@Kovensky Hopefully facebook/jsx#94 will be merged soon, so that my PR for Babel/Babylon can include those changes. :)

@clemmy
Copy link
Contributor Author

clemmy commented Oct 23, 2017

Sorry I've been inactive with this PR for a while! I'm going to open a corresponding PR for this on the Babel repository in the coming days. :)

Copy link

@sebmarkbage sebmarkbage left a comment

Choose a reason for hiding this comment

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

Looks good from a JSX spec perspective. Any more thoughts from the Babylon side?

Copy link
Member

@hzoo hzoo left a comment

Choose a reason for hiding this comment

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

looks good! thanks @sebmarkbage @clemmy!

@hzoo hzoo merged commit a1125b2 into babel:master Oct 28, 2017
@clemmy
Copy link
Contributor Author

clemmy commented Oct 28, 2017

🕺

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants