Add proposed fragment changes

[clemmy]

This is the JSX spec change for supporting <></> fragment syntax in JSX. One of the tougher design decisions on this spec was whether to go with a new node type for fragments (e.g. JSXFragment, or re-use the existing JSXElement node with an additional isFragment field. I opted for the latter because:

• a fragment has very similar behavior to a JSXElement in terms of the AST (e.g. it contains children, openingElement, and closingElement)
• the pattern of using a flag has already been adopted in the spec (e.g. selfClosing)
• this has the lowest amount of friction when integrating with the current ecosystem

To illustrate the last point, I've written forks of babel, babylon, and eslint-plugin-react to prototype, and it turned out to have very few LOC.

Some example scenarios:
< ></> // success

<></> // success

< attrib></> // fail

<>hi</> // success

<><span>hi</span><div>bye</div></> // success

<></something> // fail

UPDATE
Spec has been updated to introduce new node type JSXFragment instead of re-using JSXElement.

#84

[sebmarkbage]

The authoritative syntax spec is the README.md file. The AST.md file is just a recommended form to be used by parsers but not required.

This needs to be defined fully in README.md without referencing AST.md and currently it doesn't.

In the actual syntax spec, we don't have such a flag precedence so I think it should be its own entry point. I.e.

JSXFragment:

• < > JSXChildren_opt < / >

[sebmarkbage]

I would propose that you explore:

interface JSXFragment <: Node

interface JSXOpeningFragment <: Node

interface JSXClosingFragment <: Node

[sebmarkbage]

This type annotation is now incorrect or you need to describe what should be in here since fragments don't have an identifier nor member expression nor namespace name.

If isFragment is true this is always null, if isFragment is false then this is always not-null.

That's an indication that this shouldn't use a flag but its own interface.

[sebmarkbage]

This type is also not describing the constraints since attributes will always be an empty array.

[sebmarkbage]

This comment is not fully describing the type constraints. It's just ad-hoc. Declaring a new interface will avoid that problem.

[sebmarkbage]

Ooops. Meant to request changes.

[clemmy]

That makes sense. Do you think it'll be nonsensical to take out the isFragment flags and simply identify the fragments with JSXIdentifier whose name is ''? I guess that would still have the constraints to name being '' and attributes being [] though.

On another note, do you know how closely implementations follow specifications? I'm just wondering if I'll need to re-write my forks to follow the new spec for the first upstream into Babel.

[sebmarkbage]

They follow reasonably close and we try to adjust either the implementation or the spec to align whenever they diverge.

[clemmy]

@sebmarkbage Also, why interface JSXFragment <: Node instead of interface JSXFragment <: Expression, similar to how JSXElement is currently?

[sebmarkbage]

Ah, yea JSXFragment should probably be Expression. You're right.

[clemmy]

@sebmarkbage It seems a bit unnecessary to have JSXOpeningFragment and JSXClosingFragment since those always represent <> and </> in code. What do you think of just having JSXFragment, with the grammar being:
<> JSXChildren_opt </>
?

[sebmarkbage]

I think in the spec it is enough to just have JSXFragment. The reason you want to also have the OpeningFragment and ClosingFragment in the AST is so that you can associate meta data such as start and end column/line location information with each of those separately.

[clemmy]

@sebmarkbage Ready for review again

[sebmarkbage]

This should use the ` character around each token with a space between.

- `<` `>` JSXChildren<sub>opt</sub> `<` `/` `>`

So the formatting comes out like this:

• < > JSXChildren_opt < / >

The point of this is because we build this on top of the normal tokenizing in JavaScript. These individual tokens already exist but there's no <> token or </> token so when building a parser, you have to parse these individually. It also means that it is ok to have whitespace or even comments(!) between them.

E.g. this is valid:

let fragment = <>
< // comment
/ /* another comment */ >;

ok. lgtm. Let's get some more stakeholder looking at this before merge.

[sebmarkbage]

cc @syranide @RReverser @hzoo @loganfsmyth

[loganfsmyth]

Seems reasonable to me.

[loganfsmyth]

A JSX element consists of maybe?

[loganfsmyth]

Same as above. Feels like the grammar is weird in these.

[syranide]

We're leaving out keying for the moment? Or is there an intended alternate mechanism (special function)?

Keying is kind of important for a bunch of use-cases so I think it's important to at least have an "answer" for when the question/issue is posted.

[clemmy]

You bring up a good point @syranide. I think that key-ing a fragment is a rare enough use case that if you really wanted to do it, you would simply not use the shorthand syntax (<></>) for it, but be more explicit and use what it de-sugars to (e.g. <React.Fragment key={...}> or <#fragment key={...}>). What do you think?

[syranide]

You bring up a good point @syranide. I think that key-ing a fragment is a rare enough use case that if you really wanted to do it, you would simply not use the shorthand syntax (<></>) for it, but be more explicit and use what it de-sugars to (e.g. <React.Fragment key={...}> or <#fragment key={...}>). What do you think?

E.g. any time you have a conditional with multiple different return-values that shouldn't be reconciled, they should be keyed. So I suspect it isn't as rare as one thinks, simply because most cases goes unnoticed and is only rarely done for that reason (thus; yes rare, but needs an official story). React by chance reconciling correctly or the incorrect behavior not being visually indicated/broken. I've seen a few such posts, where they have been utterly confused because it "usually works" and "looks right".

But it's also interesting to think about this case:

foo() {
  return <>...</>;
}
bar() {
  return <>...</>;
}
render() {
  return <div>
    {cond ? foo() : bar()}
  </div>;
}

What if you don't want to be reconciled, should you as good practice key the fragments in foo/bar or should you apply the key somehow in render?

Also I wouldn't think it desugars to any of your examples but rather a pre-keyed array (or special React-method). If it desugars to something "sensible" that seems like an ok balance, but I don't think so?

PS. I don't want to stall the proposal. Keying, however it ends up being done, should be well compatible with this proposal.

[sebmarkbage]

I think the idea is that the official story is <Fragment key="...">...</Fragment> and <>...</> is short hand for <Fragment key={null}>...</Fragment>. Of course, you're also free to properly key the individual items in the fragment which also avoids the issue.

[clemmy]

@syranide If I recall correctly, if the children are different inside foo's returned fragment and bar's returned fragment, then they shouldn't be reconciled. @acdlite may be able to shed more light on that. Also, as @sebmarkbage mentioned, it will be de-sugaring into something sensible.

Something to keep in mind during our discussion is that JSX is meant to be decoupled with React, and I think reconciliation is an implementation detail specific to React.

[syranide]

@sebmarkbage I'm assuming Fragment is a reserved keyword (or a special imported class) and not just the equivalent of a regular component class (i.e. opaque)?

@syranide If I recall correctly, if the children are different inside foo's returned fragment and bar's returned fragment, then they shouldn't be reconciled.

Only if the component classes are different, e.g. if they are two different buttons (save and discard) of the same class then it could have bad side-effects when one gets reconciled with the other.

Something to keep in mind during our discussion is that JSX is meant to be decoupled with React, and I think reconciliation is an implementation detail specific to React.

EDIT: Yeah of course, but how well it works for React seems like a good litmus test.

[clemmy]

@syranide Fragment will be an opaque component class exported under the React namespace.

[clemmy]

Thanks @sebmarkbage for merging my PR! I'll update my forks to have the correct implementation of the fragment syntax, and try to get it upstream into Babel/Babylon. :)

[thysultan]

Something to keep in mind during our discussion is that JSX is meant to be decoupled with React,
de-sugars to (e.g. <React.Fragment key={...}> or <#fragment key={...}>)

How would this affect JSX in a non-React context, i.e with the pragma /* @jsx h */?

The #fragment or some kind of global symbol better favors inter-op with non-React libs.

[clemmy]

@thysultan For React, we're planning to have React.Fragment export a string, '#fragment', but we're not finalized on this yet. Inter-oping with non-React libs is definitely something we take into consideration, thanks for bringing it up! You can check out our discussion and progress here: facebook/react#10783

[uniqueiniquity]

@clemmy It may not matter, but it seems like your changes to the AST and the README disagree?
AST.md permits a fragment as a child of an element or another fragment, but the README doesn't. I saw the comments above that the README should be the authoritative source but I just wanted to check.

[clemmy]

@uniqueiniquity Ah, I think you're right -
Are you referring to adding it over here:

JSXChild :

JSXText
JSXElement
{ JSXChildExpressionopt }

[uniqueiniquity]

Yep, that's what I meant.

[clemmy]

Thanks for the catch!

[geoffreydhuyvetters]

Is this already released or do you have a version ETA? I love this :)

[clemmy]

Hey @duivvv. We're going to be officially releasing this with a blog post next week. 😄