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

Spec proposal: extending the language to allow spreading children #57

Closed
calebmer opened this issue Jun 16, 2016 · 10 comments
Closed

Spec proposal: extending the language to allow spreading children #57

calebmer opened this issue Jun 16, 2016 · 10 comments

Comments

@calebmer
Copy link
Contributor

calebmer commented Jun 16, 2016

I propose we modify the JSX specification to allow for the spreading of child nodes in JSX like so:

<div>{...children}</div>

Instead of the implicit array flattening done by React and other JSX consumers currently.

Rationale

JSX with React allows you to do this:

const TodoList = ({ todos }) => (
  <div>
    {todos.map(todo => <Todo key={todo.id} todo={todo}/>)}
  </div>
)

However, React cleverly hides what is actually happening. If we turn the above JSX into a static object, we get something that looks like this:

const TodoList = ({ todos }) => ({
  type: 'div',
  children: [
    todos.map(todo => )
  ],
})

As you can see we have a nested array:

[todos.map(todo => )]

This requires JSX consumers to flatten arrays before processing them. A step that is not only unnecessary but also difficult to express in a JSX type system that would otherwise look like a tree.

type JSXNode = JSXNodePrimitive | JSXElement

Must instead become something to the effects of:

type JSXNode = JSXNodePrimitive | JSXElement | Iterable<JSXNode>

It’s strange that we should have this problem, however, when the solution should really be in the language. Already ES6 has introduced the spread operator (...), so modifying our above example to:

const TodoList = ({ todos }) => ({
  type: 'div',
  children: [
    ...todos.map(todo => )
  ],
})

Will give us the list of list of children the JSX consumer needs without requiring a post processing step. We add the expressiveness to the language and we remove complexity from our JSX consuming libraries (like React).

In order to add this extension, we just extend JSXChild to allow for the type { ... AssignmentExpression(opt) }.

Then the above example would become:

const TodoList = ({ todos }) => (
  <div>
    {...todos.map(todo => <Todo key={todo.id} todo={todo}/>)}
  </div>
)

Or the classic children example would be:

<div>{...children}</div>

And this would compile to a form we expect at a type level:

{
  kind: 'div',
  children: [
    ...children,
  ],
}

Motivation

React isn’t the only library/framework which should be able to consume JSX, and we shouldn’t expect every library/framework to adopt React’s JSX “workaround” of flattening child arrays when in fact that is a fault in the language extension.

Therefore, in order to simplify libraries and avoid extra complexity in JSX consumers we should implement this proposal.

Next Steps

I wanted to submit this proposal here to see if anyone had opinions on it before I started writing code. In the near future I will probably submit a PR to the Babel repository modifying the babel-plugin-syntax-jsx plugin adding this change. After that PR I will implement a transform in my general JSX transformation library babel-plugin-transform-jsx. If both experiments are successful we can consider adding the child spread functionality to the JSX spec and other JSX transpilers.

Thanks!

@syranide
Copy link
Contributor

syranide commented Jun 16, 2016

This requires JSX consumers to flatten arrays before processing them. A step that is not only unnecessary but also difficult to express in a JSX type system that would otherwise look like a tree.

When is this a problem (not being snarky)? You may not modify children (it's considered an immutable opaque structure), so passing it through React.Children.toArray (or the other helpers) flattens it and returns a mutable list, other than that you may only pass it through as-is.

Also, as implemented for React, JSX compiles to a varargs function call which internally only allocates an array for children if there are 2 or more children. For zero or one child they are returned as null and only the child itself respectively, no array. Similar behavior should make practical sense for all implementations of JSX. So then <A>{...children}</A> doesn't seem at all beneficial over just <A>{children}</A>.

@calebmer
Copy link
Contributor Author

@syranide It’s important to remember that I’m not thinking about this from a React perspective. You’re right in that if JSX were solely for React this change wouldn’t make sense. However, for the greater JSX universe this could be helpful.

When is this a problem?

The problem is really noticeable when expressing a JSX structure in a type system. In the type system you now need to accommodate what is really an implementation detail in the type grammar. Of course, here I assume that there is no transformation function like (React.createElement), rather the JSX is just transformed into POJOs.

In addition, if you are going to support this pattern (which JSX is arguably not useful without the idea of an array of children) you must support a post processing layer before your JSX objects are useful. Whether that be on an element level (React.createElement) or on a tree level, some JSX processing must occur.

so passing it through React.Children.toArray (or the other helpers) flattens it and returns a mutable list

But this exactly what this proposal is trying to avoid. The need for a helper function. A helper function may make sense for React, but it adds complexity and weight to lighter implementations using JSX. JSX is very high up on the levels of abstraction, whereas React is a little lower. Ideally JSX consumers shouldn’t have to step down an abstraction level for JSX to be useful.

Also, as implemented for React…

This is a React optimization. Technical decisions made by React shouldn’t leak up the abstraction chain and block features in JSX. React users need not use this feature if it’s a React anti-pattern.

@syranide
Copy link
Contributor

syranide commented Jun 16, 2016

But this exactly what this proposal is trying to avoid. The need for a helper function. A helper function may make sense for React, but it adds complexity and weight to lighter implementations using JSX. JSX is very high up on the levels of abstraction, whereas React is a little lower. Ideally JSX consumers shouldn’t have to step down an abstraction level for JSX to be useful.

If you don't put the burden of flattening the children implicitly on the consumer then it falls on the user of JSX to pre-flatten all children. Definitely a possibility, but it's something that would make certain use-cases very troublesome. Consider <div><Header />{keyedChildren}{indexedChildren}</div>, remember that keys are meant to be local to each parent object, so even just concating all 3 "children" (which is not very nice) would not work as implicit indices would then be disturbed, you would have to explicitly convert the indices of indexedChildren to keys and also prefix to avoid conflicts with the keyedChildren (which would also need to be prefixed). So you would quickly create a bunch of helpers to deal with this right? Filtering requires that you attach explicit keys where there is none as well, that's another helper.

It's my opinion, but while that it is in some sense an implementation detail of React, it also influences the JSX syntax. There's always a trade-off between language features and runtime features. Being able to have nested maps/sets/arrays is basically an assumed feature of JSX if you ask me.

<div>{...children}</div>

Could not translate to children: [...children], because as mentioned above you have to consider that children may be maps/sets/iterators/younameit. There is no operator that can do that, so it would essentially have to compile to children: flattenChildren(children)? Unless you choose to only support arrays. But translating it to flattenChildren(children) which would have to handle keys does not seem like a valid interpretation of a ... operator, it should "just merge".

It also seems to me the only reason it would be the ... operator would be to support <div>{foo}{...children}{bar}</div>, but that doesn't seem very useful due to implicit keys. A "pass-through operator" (imagine <div>{=children}</div>) could make theoretical sense perhaps, but it seems like a weird optimization as there would never be a reason to write <div>{children}</div> then, only <div>{=children}</div>, so why wouldn't that be default?

It's late and I feel like I'm rambling a bit, but it seems to me that all this would be doing is transferring the responsibility of flattening children to the user, explicitly requiring the use of ... everywhere to pre-flatten children. You would still need the underlying helpers unless you only intend to support arrays. The intended benefits seems stripped away by the practical issues encountered?

@calebmer
Copy link
Contributor Author

calebmer commented Jun 17, 2016

If you don't put the burden of flattening the children implicitly on the consumer then it falls on the user of JSX to pre-flatten all children.

I agree. This is the intended nature. Explicit notion over an implicit conversion.

Consider <div><Header/>{keyedChildren}{indexedChildren}</div>

This is a good example, but I’d argue here that I’d rather give this case to the user to optimize instead of assuming every JSX runtime will optimize for. A user that understands their runtime would not write JSX that looks like this. In addition, this proposal would not stop React from optimizing for this case.

…you would have to explicitly convert the indices of indexedChildren to keys and also prefix to avoid conflicts with the keyedChildren (which would also need to be prefixed). So you would quickly create a bunch of helpers to deal with this right?

This makes two assumptions I:

One is that keys are array local. I know I’ve considered instead using ids as element keys because an id would serve two purposes. First to identify the element in the browser and second to optimize lists. And in this case an id must always be global.

Two is the assumption that this is a common case and people will choose to write helpers instead of nesting like so: <div><div>{keyedChildren}</div><div>{indexedChildren}</div></div>.

There are ways to work around this potential performance issue without writing helpers, and if people chose to always use global ids instead of keys in their framework the point is moot altogether.

Could not translate to children: [...children], because as mentioned above you have to consider that children may be maps/sets/iterators/younameit…

Run this code in an ES6 environment:

[...new Map([[1, 2], [3, 4]])]

The result is [[1, 2], [3, 4]]. The spread operator supports any iterator. Array, String, Map, Set, and the object returned by generators. All are iterables and all are supported by the spread operator. If React wanted to do a special optimization for Maps or for another non-iterable object it still can under this proposal. The user just shouldn’t spread. However, the lowest denominator JSX consumer doesn’t have the time or budget to support all of these cases.

Therefore the spread operator (...) does do all that and we can avoid children: flattenChildren(children) which I agree is undesirable.

It also seems to me the only reason it would be the ... operator would be to support <div>{foo}{...children}{bar}</div>, but that doesn't seem very useful due to implicit keys.

Yep 😊

And again, keys are an implementation detail. They are not part of JSX. If we are going to weigh implementation details in deciding whether to adopt the proposal we should weigh the added complexity of requiring users to flatten JSX children. We can solve key problems. Either by using global ids, educating developers, or by declaring this an anti-pattern for specific frameworks. However, other JSX consumers cannot solve the problem of not needing to flatten children always without this proposal. On balance, I think that’s in favor of adopting this proposal.

It seems to me that all this would be doing is transferring the responsibility of flattening children to the user, explicitly requiring the use of ... everywhere to pre-flatten children.

Yes. You can’t do:

const foo = [1, 2, 3]
const bar = [4, foo, 5]
assert(bar === [4, 1, 2, 3, 5])

So developers are already expected to spread lists in an array. This proposal is a natural extension of this.

You would still need the underlying helpers unless you only intend to support arrays.

(See my above statement on iterators)

The intended benefits seems stripped away by the practical issues encountered?

I don’t think so, I only think the practical issues are for React. JSX isn’t exclusively for React (or at least I hope).

If this proposal gets implemented in babel-plugin-syntax-jsx, the React team still has the ability to disallow it. A warning (or error) can be added in Eslint or even in babel-plugin-transform-react-jsx itself. This proposal is for the broader community of JSX consumers, not so much for React.

@syranide
Copy link
Contributor

[...new Map([[1, 2], [3, 4]])]

Ah, my bad :)

Anyway, I can imagine some alternate use-cases where it makes sense to allow the user to produce children output without nesting (to produce exact data structures) and it seems like a natural and harmless extension in itself, so IMHO it's seems sane as presented and fine to me (but it's not my decision).

So have a 👍 from me. I imagine your best bet moving forward would be to just implement/fork the babel extension and see if you can drum up support that way. It's always easier pushing something through if you can prove there's actually a real-life benefit/demand here.

@syranide
Copy link
Contributor

cc @sebmarkbage If you have the time, any thoughts?

@sebmarkbage
Copy link
Contributor

I always intended for opening up this syntax. It is the reason spread of attributes is {...attrs} instead of ...{attrs}.

I'd be fine supporting this in the JSX syntax specification but disallowing it in the React implementation, or simply treat it the same as {children}.

The key issue very complex and way beyond the scope of this thread, but needless to say, if we thought any of the proposed alternative were better than the complex nested data structure we'd prefer it.

here I assume that there is no transformation function like (React.createElement), rather the JSX is just transformed into POJOs.

Note that createElement doesn't really do anything that can't be statically known other than the defaultProps. So the idea is that you should be able to translate these into POJOs even for React.

I'd love to get to a point where implementations can agree on a format of this POJO but this is probably a key point preventing this from happening (pun intended).

@calebmer
Copy link
Contributor Author

calebmer commented Jun 22, 2016

Opened a PR in babel/babylon#42 to implement this functionality. @kittens wants to see it properly specced in this repo first which is understandable. Do you want me to proceed with a PR to this repo, or would you like to see the babylon fork get some use first?

@sebmarkbage
Copy link
Contributor

A PR here seems fine. Seems pretty non-controversial.

@sebmarkbage
Copy link
Contributor

Done.

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

No branches or pull requests

3 participants