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 JSX Fragment syntax support #6552

Merged
merged 9 commits into from
Nov 3, 2017
Merged

Conversation

clemmy
Copy link
Contributor

@clemmy clemmy commented Oct 24, 2017

Q                       A
Fixed Issues? No
Patch: Bug Fix? No
Major: Breaking Change? No
Minor: New Feature? Yes
Tests Added + Pass? Yes
Documentation PR Included
Any Dependency Changes? No

This is the corresponding PR for babel/babylon#755 in order to support JSX fragment syntax (facebook/jsx#93).

@clemmy clemmy changed the title Add jsx fragment syntax Add JSX Fragment syntax support Oct 24, 2017
@@ -10,6 +10,7 @@ type ElementState = {
call?: Object; // optional call property that can be set to override the call expression returned
pre?: Function; // function called with (state: ElementState) before building attribs
post?: Function; // function called with (state: ElementState) after building attribs
compat?: boolean // true if React is in compat mode
Copy link
Member

Choose a reason for hiding this comment

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

tagName: ? ^ too?

Copy link
Member

Choose a reason for hiding this comment

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

What exactly is compat again? If it's just in an older version of React maybe we should just drop it?

Copy link
Member

Choose a reason for hiding this comment

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

compat is probably a bad name but it's an all lowercase tag name (it's how you write DOM tags in ReactDOM)

The check itself is !!tagName && /^[a-z]|-/.test(tagName)

Copy link
Member

Choose a reason for hiding this comment

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

Oh, wait, this is actually unrelated to isCompatTag; it's for the react-jsx-compat transform 🤔

Turn JSX into React Pre-0.12 function calls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm 👍 for dropping the compat option. Re-visiting the README, I'm unsure of why pre, post, and compat are inside the ElementState type. These 3 are keys that should appear in options (which aren't typed). I'll reflect the changes in a commit.

exit(path, file) {
if (opts.compat) {
throw path.buildCodeFrameError(
"Fragment tags are only supported in React 16 and up.",
Copy link
Member

Choose a reason for hiding this comment

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

Should we do a peerDep on react @babel/react? (maybe for the preset)

Copy link
Member

Choose a reason for hiding this comment

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

I think Fragment name is also only exported in 16.1, even though it's technically supported in 16.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do you mean by a peerDep on react @babel/react?

Copy link
Member

Choose a reason for hiding this comment

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

I mean should @babel/preset-react have a peerDep on react or react-dom?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, good question. Would non-React library authors use @babel/preset-react? It seems like enabling @babel/preset-react and providing pragmas allows Babel to compile to alternative JSX rendering libraries.

Copy link
Member

@hzoo hzoo Oct 30, 2017

Choose a reason for hiding this comment

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

Ah yeah they may just use the specific transform itself, not the preset

@@ -9,16 +9,16 @@
**In**

```javascript
var profile = <div>
Copy link
Member

Choose a reason for hiding this comment

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

So we are changing all the examples like this? May need to put somewhere that it's only in React 16, etc as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I split it into a second example (showcasing fragments specifically and noting it's React 16)?

Copy link
Member

Choose a reason for hiding this comment

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

@clemmy yeah, let's split them up!

@@ -92,3 +92,26 @@ export function JSXClosingElement(node: Object) {
}

export function JSXEmptyExpression() {}

export function JSXFragment(node: Object) {
Copy link
Member

Choose a reason for hiding this comment

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

should have a test for printing too

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added printing test in this commit: eef5e94

Wondering why the test folders are prefixed with XJS instead of JSX. 🤔

Copy link
Member

@hzoo hzoo Oct 30, 2017

Choose a reason for hiding this comment

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

funny, 😄 that used to be a thing, can ask the team - we should rename in another pr

https://github.com/facebook/jsx/pull/26/files

@existentialism
Copy link
Member

@clemmy a rebase should get rid of the syntax-related errors since we updated Babylon in #6587

@existentialism existentialism added area: jsx PR: New Feature 🚀 A type of pull request used for our changelog categories labels Oct 30, 2017
@babel-bot
Copy link
Collaborator

babel-bot commented Oct 30, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/5556/

@clemmy
Copy link
Contributor Author

clemmy commented Nov 2, 2017

@existentialism @hzoo @Kovensky Ready for review again. 🙂

// function called with (state: ElementState) after building attribs
},

compat?: boolean // true if React is in compat mode
Copy link
Member

Choose a reason for hiding this comment

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

true if transforming in React < 0.12 compat mode?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct. IIRC, I just stuck this in there so it would throw an error if you're trying to use fragments with the plugin-transform-react-jsx-compat transform.

@@ -109,6 +174,12 @@ Replace the function used when compiling JSX expressions.

Note that the `@jsx React.DOM` pragma has been deprecated as of React v0.12
Copy link
Member

Choose a reason for hiding this comment

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

This probably just throws in React 15.4 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that something to address in this PR?

Copy link
Member

@Jessidhia Jessidhia Nov 2, 2017

Choose a reason for hiding this comment

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

Sorry, not really, was just thinking out loud 🤔

.split(".")
.map(name => t.identifier(name))
.reduce((object, property) => t.memberExpression(object, property));
};
Copy link
Member

Choose a reason for hiding this comment

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

This looks suspiciously like t.buildMatchMemberExpression

(we really need to document that in handbook 🤔)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, nevermind, it's its inverse 😆

@@ -1076,6 +1076,18 @@ Aliases: `JSX`, `Immutable`

---

### jSXClosingFragment
```javascript
t.jSXClosingFragment()
Copy link
Member

Choose a reason for hiding this comment

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

I'd actually prefer to name them jsxClosingFragment, jsxElement, etc, but the pre-existing stuff is already using the weird "SX" prefix so can't help it 😛

Copy link
Member

Choose a reason for hiding this comment

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

Separate issue 😛 , yeah whatever is doing that logic needs to handle TS/JSX..

Copy link
Member

@existentialism existentialism left a comment

Choose a reason for hiding this comment

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

Great stuff @clemmy!


export function JSXFragment(node: Object) {
const open = node.openingFragment;
this.print(open, node);
Copy link
Member

Choose a reason for hiding this comment

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

Since open isn't used anywhere can inline (i'l fix it), same as closing

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.

amazing work, great first PR 😎

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: jsx outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants