API for accessing props.children correctly

[chenglou]

(Check the comment on http://facebook.github.io/react/tips/children-props-type.html)

I also feel that saving an array allocation once out of x times causes a bit too much inconvenience. I think we should either return the array all the time or, as @vjeux suggested in IRC, expose an API for getting the children correctly.

1. Returning the array: treat one item the way way we treat multiple items.
2. Exposing an API: while we're at it, flatten the children to one single level and return it.

[vjeux]

FYI: Internally there's

• flattenChildren but that returns a map: https://github.com/facebook/react/blob/bdf2a9bb124b8fc34120949b322202497741f239/src/utils/flattenChildren.js
• mapAllChildren that iterates but doesn't create an array: https://github.com/facebook/react/blob/bdf2a9bb124b8fc34120949b322202497741f239/src/utils/mapAllChildren.js

[chenglou]

To continue the discussion: personally I'm more for returning the array. Right now it returns the first level of components (i.e. the parent's direct children) and I've found it useful in the past to retrieve the length for some computation. flattenChildren returns every child in a flat array; I feel that how the children organize themselves internally should be an implementation detail and shouldn't be leaked to parent through flattenChildren; plus, accessing props.children avoids the speed and conceptual overhead of a new API, since all wrapper components already use {this.props.children}. Though there might be a use for retrieving flattened components that I might not have thought of. @vjeux might have more to say on this.

[andreaferretti]

+1 for returning the array in each case. A rather insignificant performance optimization should not be the basis for making an inconsistent API. It is just confusing and will byte every beginner user of the library (and even some more expert ones) for no good reason

[petehunt]

I think we should expose mapChildren() more obviously and document it

[chenglou]

Wait, so what's the use-case for getting every leaf child? The problem of the current API & getting direct children would still be unsolved if mapChildren is exposed.

[andreaferretti]

In my opinion, exposing a different API to walk the children does not solve the problem. The issue is not that an API to walk the children is missing. As I see it, the problem is that this.props.children is inconsistent; even if alternate APIs exist, this will not prevent people from using this.props.children and find unexpected results.

[swannodette]

Not sure if this throws more confusion into the mix but Om actually sets this.props.children to a function which is much more convenient for our use case - it makes children computation even lazier and simplifies some design things on our end. So my initial vote is to leave this.props.children alone, and provide some higher level API and amend the documentation to recommend it.

If people find this.props.children fundamentally problematic I hope React can support some way for children to be arbitrary for libraries like Om.

Side note, as someone new to this issue, this ticket is missing quite a bit of context. Can someone explain what problem this is actually solving? Is it just a matter of convenience for some use cases?

[petehunt]

Yes, mapChildren() is an accessor similar to how Om works. It just iterates over all immediate children and ensures that any React components returned are keyed correctly.

Most of the time you don't need any other functionality. However, I think that we need to expose this method in the API -- perhaps on ReactCompositeComponent.

this.props.children has always been intended to be an opaque object. There are two main reasons for this:

• Performance: treating this as insignificant may lead to a death-by-papercuts effect, where every minor memory optimization we forego will add up and we'll wake up one morning and be hanging large apps on lower-end phones. It's far easier to code with this in mind from the beginning rather than track it down later, especially since we don't have to fundamentally change the coupling between components. Also, I have a dream of zero-allocation render() methods to avoid the GC and this would be a step in the wrong direction.
• Implementation flexibility: the way reconciliation works should always be an implementation detail. And in order for us to improve it we may need to change the data structure backing this.props.children (i.e. there is a linked list impl of React that is rumored to be 10-15% faster). If you assume this.props.children to be of a specific type then we'll either need to break apps in the future or build a compatibility layer.

So I think an optional accessor function is the right way to go, and it's already in the repo: ReactChildren.map(). Does this solve your use case?

[chenglou]

@petehunt doesn't mapChildren iterate through every leaf child rather than just direct children?
If we make this.props.children opaque and expose a method that maps direct children, then I'm good.

Edit: seems like it maps through direct children alright, nvm this.

[swannodette]

@petehunt I don't see how mapChildren() applies here. We don't iterate through children. The only thing I would like to see is that we can continue to compute children lazily.

Another side note, I also think that having opaque values on this.props was a strange design decision given that's where people put their other data.

[plievone]

seems like it maps through direct children alright

Docstrings for mapChildren (and forEachChildren) mention leaf childs and call traverseAllChildren, so it is quite confusing. Docstring also claims returning an array ("mirrored array with mapped children") even though it returns an object.

Also in terms of memory perfomance I was under impression that this.props.children being not always an array is not so much about user defined components, but mostly due to there being lot's of single native components on the page, such as (wrapped) text nodes, and avoiding creating those arrays (on each virtual dom render?) were beneficial.

[swannodette]

@petehunt just adding a few more thoughts here related to children & performance. I think it's important for React to support people who want to drive the algorithm from the outside like Om. It's unlikely that React can ever compete with Om since in the Om model we already always know precisely what data changed and already know exactly what should or should not be re-rendered. Because of JS semantics this likely will never be true in the general case for regular React users.

We could in fact deliver ridiculous low allocation between renders today if React provided the right hook, i.e. shouldChildrenUpdate. So while I think React should do what it needs to do deliver optimizations to JS users, it should also not get in the way of other systems which can deliver far more aggressive optimizations.

[petehunt]

@sebmarkbage definitely needs to be on this thread since we used to flatten children to an array (and therefore this.props.children was always an array) and we eventually reverted it.

@swannodette I don't think something like shouldChildrenUpdate() would be controversial. We used to have a magic prop called isStatic which would do exactly what you're talking about. I think the challenge here is just to make the API nice and constrained. @jordwalke what do you think?

[petehunt]

@andreaferretti can you describe your use case in a little more detail?

[andreaferretti]

@petehunt My point is that I do not have a specific use case. My concern is that an API that sometimes returns an array, sometimes a single object, is mislieading, whether or not alternate APIs exist. Even if there are alternate means to get access to the children, some people will use this.props.children and probably get unexpected results.

In other words, the probably negligible loss of performance due to an allocation of an array of a single element is more than repaid by the corresponding gain in consistency and uniformity. Why one would need a short article in the docs just to explain that an API does not behave in the obvious manner?

[sebmarkbage]

@swannodette We already consider immutable objects to be a critical part of React. Our future design will continue to revolve around optimizing the reconciler for the immutability. In fact, we're just about to land another optimization that breaks certain mutability use cases. The shouldComponentUpdate hook is currently not doing this by default but it's a critical feature that doesn't work well with mutability. Supporting a mutable JS paradigm for certain components doesn't limit our ability to optimize for immutability for larger sections.

In our next release we won't reconcile children that have the same instance as before, by default. We can assume that they haven't changed. The rationale is that if you mutate something on a prop, you also tend to recreate that child. That means that subtree updates that receive children from higher in the tree won't reconcile all the way down. This will work for free if you do subtree updates.

Is your issue that you're reconciling from the top and want to avoid rerendering the parents?

I'm not sure if I interpret your proposed shouldChildrenUpdate() API correctly. I can think of a few issues. Children are not the only way to pass components. Often you should be passing them through named properties which indicates the intension more clearly. Especially if there are multiple sets of children. There is no special case for children in composite components. You also need to be able to nest children so how do you specify this for a nested set? It also needs to make sense for fragments, i.e. returning multiple components at the top level of a single render function.

One of the important ways to do perf optimization is to create smaller components. I.e. if your children tend to update separately from the rest of your component, you should be breaking them apart anyway. Sometimes this means passing them in from above. Sometimes it means wrapping them in another Component. That other component will have shouldComponentUpdate on it so it should never needed to recreate the large list of children. Similarly, if the children are passed through above, and state updates in the current component, they won't reconcile by default.

The reason children goes on props is because there's nothing special about the children property. That's just how we express the children property in terms of the XML like syntax in JSX. Since another component can take an opaque set in named property.

var Foo = React.createClass({
    render: function() {
        return <Bar><A /><B /><C /></Bar>;
    }
});

var Bar = React.createClass({
    render: function() {
        return <Baz header={this.props.children}><div>sdfg</div></Baz>;
    }
});

If you want to pass a function to lazily instantiate children, that's perfectly valid. You will always be able to pass that explicitly.

<Baz children={fn} /> or Baz({ children: fn })

The additional arguments to the function just happens to create an opaque set.

As for why this set is opaque and why it changes shape... We're aware that this is inconvenient and I tried really really hard to get this addressed before we open-sourced. It turns out to be very tricky to solve the semantics and retain performance.

The fact that it's sometimes a single child and sometimes an array is just a perf optimization. Semantically they're equivalent and we could easily just wrap the single child in an array if that was the only issue.

The real issue has to do with nested fragments.

var a = <Foo />;
var b = <Foo />;
var earlierDefinedSet = { a, b };
<Foo>
  <Prefix />
  {this.props.dataList}
  <Infix />
  {this.props.children}
  <Suffix />
  {earlierDefinedSet}
</Foo>

We want the ability to conveniently be able to prepend and concatenate sets of components. You can't just flatten one of these to an array, because you would lose the key context. The keys can only be guaranteed to be unique in the set where they were originally passed in. Not in a concatenated set.

The initial proposal added a context to the key of a component as it passed along. To get the implementation details to get that right ended up being fairly complicated because you can extract a component from an array and place it in a new place. Ideally it would still preserve it's identity. That worked with the exception that you couldn't place the same component in two places (which you still can't).

In the end we reverted the whole thing because of the complexity of the implementation.

Another proposal was to make it truly an opaque set where you could never extract values on it. Just operate on it with custom functions. I was against introducing a new set type because we want to be able to interoperate well with a larger JS ecosystem.

With ES6 stablizing there are few alternatives that could be possible.

There was previously a strongly held believe that arrays must be treated just as any other key/value object. I believe that to be incorrect since in ES6 they can equally be treated as iterable types, which could shift the semantic meaning of an array within one of these fragments.

We could potentially expose children as a Map which observably preserves order in ES6. The contextualized keys would be part of the Map's keys. It's also possible to implement this lazily. This is probably good idea to start with.

But it still leaves a lot of unknown questions.

The same question remains about what you do with an instance that you gain access to from one of these Maps. When you place it in a different position from before, does it retain state? The caller would assume that it would. The final position in the tree may be different. You would also be responsible for preserving the key correctly. We'd probably need the ability to move nodes between parents to preserve the state intensions of the caller.

What are you allowed to access on the instance? You shouldn't be calling methods on the instances because they're not stateful and we would even want to prevent that in the future for other reasons. Even if you could call methods on children, you can't rely on a composite having those same methods. That would mean limiting the ability of your parent to wrap parts of it's children into composite. That's a really important design prinicipal.

You could possibly want to have the parent inject properties into your children but those may pass through a composite that isn't aware of those.

I think there's a larger issue about how a parent communicate with children that it didn't create itself. One way to solve this would be to expose an API that lets the parent resolve children down to an abstraction that it can understand and communicate with using a custom API.

We need to figure out what those semantics should be and then we can talk about what the type should look like.

[swannodette]

@sebmarkbage thanks for the long reply clarifies many things with respect to the current design.

I'm not sure if I interpret your proposed shouldChildrenUpdate() API correctly. I can think of a few issues. Children are not the only way to pass components. Often you should be passing them through named properties which indicates the intension more clearly. Especially if there are multiple sets of children. There is no special case for children in composite components. You also need to be able to nest children so how do you specify this for a nested set? It also needs to make sense for fragments, i.e. returning multiple components at the top level of a single render function.

This is not how Om works. You only pass data, never components. We never use React subtree updating. Everything is data-driven and all performance is derived from reference equality checks all the way down. This means you can make a composite data structure out of different pieces of data which represent different Om components - re-rendering from the root will still be lightning fast.

Thus we don't need (or really want) any of React's high level component semantics for users, we just want to drive the React algorithm. So while shouldChildrenUpdate might seem less useful to standard React users who've bought into the high level semantics it's critical for people who want to drive the system from outside who don't need them or want to actively avoid them.

shouldChildrenUpdate could take keys of changed children. This would allow you all to keep control over the underlying representation.

Speaking of which, given that you all already allow users to specify key, why not allow them to supply nextKey? Seems like this would allow users to express concatenation and prepending and you guys can make it screaming fast? :)

[swannodette]

Oh and as far as nested children and shouldChildrenUpdate - shouldChildrenUpdate is intended to recursively guide what the reconciliation algorithm should look at, nothing more.

[thSoft]

+1 Totally agree with @andreaferretti.

[plievone]

@chenglou Fixed by 945f788 and #1019 ?

[chenglou]

Eh, I guess... personally I dont feel API like onlyChild solve the problem. Sure, now we have an explicit way of retrieving the child, if we know that there's only one. But the point is we often don't.
I'm more or less ok with the API but I'll close this if @swannodette is fine with it.

[swannodette]

Just to clarify, so the idea is that we can still set children to a function and then call onlyChild to get it?

[sebmarkbage]

onlyChild should only be used on Components (soon to become Component Descriptors).

You can still set the children prop to a function and just assume that it's a function, and call it as a function.

I wouldn't do that with the second argument to the component constructors though (that translates to the children property). I.e. the "children" position in JSX.

@swannodette Do you need to use the second argument or can you use an explicit property named children?

[swannodette]

Currently we are using the 2nd argument to component constructors, however we can easily switch to doing this via props.children. If this works, then I'm fine with closing this one. Thanks all!

[slorber]

is onlyChild documented somewhere?

[chenglou]

http://facebook.github.io/react/docs/top-level-api.html#react.children.only

[slorber]

oh thanks, it has been renamed to React.Children.only I guess

[sophiebits]

Sorry, to clarify – onlyChild is the internal name; React.Children.only is the public one.

[darkyen]

Can I traverse them recursively ? This doesn't seem to work

   function visit(node){
          return ReactChildren.map(node.props.children, visit);
   };

    var drawStack = visit(this);

[binarykitchen]

Is there a function that returns me all the React children recursively?

I need that for collecting and adjusting form data before submit.

[warmhug]

I want to traverse all children recursively, find the specific type of node and use React.cloneElement to change their props in some condition. If this is a wrong method?