Take children off props

[jimfb]

• Having children on props is not only a bit awkward/random as per prior conversations with @jordwalke (ie. why is the prop "children" special cased?). Not a deciding factor, but certainly a bit irksume from a theoretical computer science perspective. element.children feels cleaner than element.props.children.
• The current API is a bit ambiguous with regards to the result of React.createElement('div', {children: ...}, ...);, since children is now specified with two potentially conflicting values. Imagine that the third argument is null or [] or just a different value. I don't actually know what would happen off the top of my head, especially in the null case (is the null treated like undefined or does the third argument override the second?)
• Having children on props means that the props object saved in the ReactElement can't be hoisted up by an intelligent runtime, which would otherwise be possible for runtimes/transpilers that can see that the non-child props don't depend on that particular render. This could save thousands of allocations per render.
• Most importantly: String compares are expensive. Because children occurs in props, it means we need to do an additional string compare for each property we iterate over, since we need to handle the children prop as a special case.

This is mostly an RFC issue, it's something to consider before doing inline elements. cc @sebmarkbage

[syranide]

This seems odd to me, I agree that the way children is currently implemented is weird, but IMHO that's not because it doesn't belong in props. It's because React.createElement provides the convenience of specifying children through varargs, which to me is a convenience provided for non-JSX users but otherwise not a good API decision.

If you forget about JSX for a second then all that children comes down to is a formal name for a commonly used property (which JSX has user-friendly sugar for), other than that children is a prop just as much as everything else, pulling it out of props should realistically mean that shouldComponentUpdate now has to compare non-props too, that seems odd. (EDIT: Also, in the context of some components housing multiple containers for children, it's nice that children works identically to childrenA and childrenB except for there only being no JSX-sugar for it. So I'd rather see children being not special rather than more special.)

So to me it sounds like all this boils down to the API decision of allowing children to be provided through varargs has significant negative side-effects that we don't want affecting JSX users, so it should move to a helper, not that children doesn't belong in props... ?

EDIT: So from a technical perspective I can't think of any reason to make children special, but from a user perspective it makes sense (that's why we have JSX-sugar).

[jimbolla]

this.props.children also makes it awkward/difficult to implement componentShouldUpdate(). It's possible to treat all other props as immutables and therefore do shallow compare, but children are mutable react components, meaning they have to be treated differently. I have yet to come up with a decent way to implement componentShouldUpdate() on a component that accepts children.

[jimfb]

@syranide It's not just createElement that's weird. Even for JSX users, they can specify children as a prop on the jsx element (possibly via ellipsis) and simultaneously provide child nodes in the JSX element. You have the oddities for both jsx and non-jsx users alike. Changing the createElement signature doesn't solve the weirdness for jsx users.

But that's not even the most powerful argument. The most powerful ones are both technical:

• Both JSX and non-JSX users are taking a perf hit, because every prop key needs to be checked to see if it stringcompares to the string "children", because we need to skip "children" when rendering node attributes. This means we do a needless stringcompare for every prop on every element for every render.
• And as if that weren't bad enough, we take another perf hit. Having children on props means that a super common case (an element with static props but non-static children) can't have the props object hoisted up out of render. This results in thousands of needless allocations (and the associated garbage created) for a single large render. So there is a second perf hit.

@jimbolla Yes, componentShouldUpdate is hard to implement if a component accepts children. This doesn't really solve that problem. But yes, I agree, processing children in componentShouldUpdate is another case where they often need to be treated differently.

[gaearon]

It's not just children. A <Layout> component might accept sidebar and content props which are elements. If we want to treat children specially for prop hoisting optimization, what about sidebar and content? Should they, too, no longer belong to props?

Edit: just realized that's what @syranide said above. :-P

[gaearon]

It's possible to treat all other props as immutables and therefore do shallow compare, but children are mutable react components, meaning they have to be treated differently.

Nit: they are not mutable, it's just that they're usually generated inside render, and thus new every time.

Babel hoisting will help this, but I'm not sure how well. Whereas the whole <A><B><C toggle={true}></B></A> element tree could be hoisted as a constant, <A><B><C toggle={this.props.toggle} /></B></A> will (in my understanding—correct me if I'm wrong) cause C, B, and A to be non-hoistable.

That's the problem I think @jimfb is describing, am I being correct?

[jimfb]

@gaearon Correct on all counts. The props of B and A would be hoistable if it weren't for the fact that C becomes a prop. But don't forget about the stringcompares, which are equally important/expensive.

[sebmarkbage]

irksume from a theoretical computer science perspective

What does that even mean? Care to be more specific? :)

this.props.children isn't the weird part. It's this.props that is weird. In normal OO you would just put everything on this. E.g. this.children, this.style, this.className. The right mental model is to think of props as the main object (this) and in the functional style we'll just get rid of this completely.

The only thing that makes this weird is because of XML and specifically the XML object model.
The XML model special cases child elements in its parsing. However, a deserialized DOM is represented by .propertyName and .childNodes. They're on the same object.

The only thing that screws this rule up is that the DOM also allowed access to the serialized attributes through .attributes and getAttribute() which is nested.

If anything, if we want to be consistent we would add this.props.attributes to give you a reflective API of the original source text. Ofc, we won't do that. This is generally considered a mistake to expose the serialized form in the object model.

I think that it is this mental connection to "attributes" that trips people up. However, it is also the most broken part of the DOM and something we've tried hard to get away from.

So, no, from a "theoretical computer science perspective" the current model makes as much sense.

Another thing that trip people up is that children are not "instantiated" before their parent. Meaning that you don't have access to the instances. The general misconception around children is related to that and not the nesting.

E.g. people expected to be able to treat this.children[0] as if it is a ref. This is how you would do it in the DOM or XHP, and many other traditional models. However, React doesn't work like that and it gives us many important/interesting opportunities. Even if we added the elements to this.children it would be confusing because of this difference.

We might want to have that ability to pre-instantiate children at some point though. See for example: https://github.com/reactjs/react-future/blob/master/04%20-%20Layout/02%20-%20Layout%20Components.js#L23 I wanted to keep that open name for that potential purpose or something like it.

Even for JSX users, they can specify children as a prop on the jsx element (possibly via ellipsis) and simultaneously provide child nodes in the JSX element.

This is theoretically confusing if you're trying to do this. However, think about it, why would this happen in practice? What would the user do to end up in this situation?

If you try to do this, while intentionally knowing that they're the same namespace, you might not know which priority or merging behavior applies, so you just avoid it.

If you accidentally do this because you assume they're different namespaces, then you're probably not targetting HTML (since children is not a valid attribute). So you're trying to build a composite component, but then when you try to use the prop you realize that there is no way to access them as separate things.

Besides, we could simply lint for this, or even disallow it in the transpiler.

Having children on props means that the props object saved in the ReactElement can't be hoisted up by an intelligent runtime

That's incorrect. The ReactElement will never be hoistable if you have dynamic children. You could potentially hoist the secondary props object but you will need the wrapper object around them.

This is the issue that @jordwalke has pointed out in the past. However, it is unclear how often this would actually be beneficial and how large the impact would be since you have wrapper objects around things anyway. Your maximum saving would be n / 2 - 1 for the rare case that it is actually possible to hoist. So it's not like you would be able to hoist the whole tree anyway. Being able to hoist is rare because of things like props being passed and event handlers.

We have the same issue with style, so should we just move that special case up to ReactElement too? What about data? Where does it end? We could just merge it all onto ReactElement but then it would be a pain to use spread (...) etc. to forward properties, which is also true for children.

Besides, if we end up using value types for props, they'll likely just merge onto the same allocation as the outer ReactElement allocation anyway. Think struct fields in C++ or C#.

Because children occurs in props, it means we need to do an additional string compare for each property we iterate over, since we need to handle the children prop as a special case.

Iteration and reflective APIs are expensive and does not JIT very well. So the solution is to try to avoid that as much as possible anyway. See for example:

#3227

For known props on HTML elements I always wanted to create a function that reads all the props instead of iterating. E.g.

function update(oldProps, newProps) {
  if (oldProps.foo !== newProps.foo) {
    element.setAttribute('foo', newProps.foo);
  }
  if (oldProps.bar !== newProps.bar) {
    element.setAttribute('bar', newProps.bar);
  }
  // ...
}

processing children in componentShouldUpdate is another case where they often need to be treated differently

That doesn't solve the sidebar and content scenario. You just need to do a shallow compare on any known children-like properties. That's not just the case for children but style and any other object that is likely to be recreated during a rerender of a parent. The point of shouldComponentUpdate is to bail out on whole subtrees so it wouldn't make sense to just skip one component while still proceeding to its children. We also don't have any way of doing that for composites since we don't know which property path corresponds to the next set of props for the child.

For these reasons, we've rejected this proposal in the past and there's no new knowledge here so I'll close it out. Maybe if we get some actual numbers on perf impact in real apps we could reconsider if we don't think that alternative compilation strategies (e.g. value types) are likely to be realized soon enough to warrant a massive codemod/upgrade path.

There is, however, another reason we might want to do this. We might change this as a compromise / concession if we try to standardize the ReactElement data structure as part of JSX. We don't really care about preserving XML's idiosyncracies but it is likely that other people using JSX to target XML might want to do that.

If we ever get to a point where we can join others in standardizing JSX's data structure, then they might want to special case children and events. However, that is a big maybe and there are other quirks like key and ref semantics that have to be dealt with first.

[jimfb]

@sebmarkbage

irksume from a theoretical computer science perspective

What does that even mean? Care to be more specific? :)

It's odd that <MyClass childNodes='foo' children='foo' childElements='foo'>bar</MyClass> would have the children property randomly overwritten by 'bar'. Why is it overriding children and not some other prop (like childNodes or childElements). It just feels dirty/arbitrary, making props feel a bit impure.

That's incorrect. The ReactElement will never be hoistable if you have dynamic children. You could potentially hoist the secondary props object but you will need the wrapper object around them.

The ReactElement will never be hostable, but the props object is hoistable, and that's what I'm talking about in this issue. The keys of ReactElement are uniform and thus much more optimizable, so hosting the element is less important anyway; the critical part is hoisting the props.

We have the same issue with style, so should we just move that special case up to ReactElement too?

Maybe we should, yes! Or stop treating style as a special case (object instead of string), but that's a different discussion. In practice, the style objects are oftentimes constant and thus already hoistable.

For known props on HTML elements I always wanted to create a function that reads all the props instead of iterating.

That solves the update props but not initial markup/render.

Maybe if we get some actual numbers on perf impact in real apps we could reconsider if...

On a real component rendered by a real github user (chosen more or less at random), taking children off props and hosting the props object improved initial render time for my SSR renderer by more than 10%. It brought a 36ms render down to a 29ms render. Not a huge improvement, but very measurable, and the milliseconds add up. This was on a different JSVM (ie. not V8) so the relative costs could be a bit different and numbers should be taken with a grain of salt, but still, it's a valid data point.

to warrant a massive codemod/upgrade path.

The relative size of a codemod is debatable, depending on how far we would want to take such a change. It only NEEDS to affect the representation of inline elements and the OUTPUT representation of React.createElement(). We COULD change the public component API of ReactComponent, but this is not required to realize much of the performance benefit here.

[sebmarkbage]

That solves the update props but not initial markup/render.

For a known whitelist it still works, and for the __t optimization it works without a whitelist for elements that all use the same property names, which for HTML nodes is frequent.

We COULD change the public component API of ReactComponent

If we change the inline representation, or output of createElement we would have to create a new object that gets passed into the public component API which would be much slower. Therefore, I think we would HAVE to change the public component API. I'm not sure what solution you're proposing? Do you mean special casing the "string" type element so that only native elements have this property?

On a real component rendered by a real github user

Can you point us to it? Is this a complex component or one that gets rendered a lot of times?

Also note that hoisting isn't free. We might need to lazily initialize the elements since otherwise we can negatively impact the load time of the modules, or come up with another optimization. That's probably something can overcome though.

[jimfb]

@sebmarkbage

For a known whitelist it still works, and for the __t optimization it works without a whitelist for elements that all use the same property names, which for HTML nodes is frequent.

You're still iterating to generate the markup, unless I'm failing to understand/see something (which is entirely possible).

If we change the inline representation, or output of createElement we would have to create a new object that gets passed into the public component API which would be much slower.

Well, it's a dirty hack, but we could mutate the props before passing them to the component and then mutate them back when we're done, maintaining the illusion that ReactElements are immutable for the lifespan of the element.

Can you point us to it? Is this a complex component or one that gets rendered a lot of times?

It's the component discussed here: #2608

I've found it to be a particularly good one to benchmark against. I got my SSR render time down to 29ms, but without a couple of features (like context) which are not used here, and using some black magic like aggressively rewriting ReactElement.