Children don't re-render when parent component is optimised

[benoitgrelard]

I am seeing something that looks like a bug and have written a simple test case for it.

Please refer to this WebpackBin as it contains the test scenario:
http://www.webpackbin.com/EJpabsuJW

[jimfb]

This is a usage question, rather than a bug in React. Your shouldComponentUpdate function is explicitly telling React that it should not update, so obviously it does not update, this is expected behavior.

Optimizing components that can take in arbitrary children is somewhat non-trivial. In general, it will require walking/diffing the children in some way, and (for completely general children) this is technically not possible for the same reasons that componentWillReceiveProps does not guarantee props will be different (http://facebook.github.io/react/blog/2016/01/08/A-implies-B-does-not-imply-B-implies-A.html).

Also, keep in mind that shouldComponentUpdate should be used somewhat sparingly in critical parts of your application. Diffing values (especially a tree of values, like children is likely to be) can be expensive, and if your shouldComponentUpdate is ultimately going to return true anyway, than React will need to do that same diffing again during reconciliation, so you're just wasting CPU cycles (ie. it might actually be cheaper as a whole to just skip implementing that function).

Anyway, I'm going to close out the issue, since it's not a bug in React. Feel free to continue the discussion on this thread, or move the discussion to StackOverflow.

[gaearon]

This might be an inconvenience or even a conceptual problem, but definitely not a bug.
As you note here:

  shouldComponentUpdate(nextProps) {
    // naïve, only for demo purpose
    return nextProps.color !== this.props.color;

    // how to optimize this for children??
    // it doesn't make sense for it to have to check for its
    // children, as it's simple composition, it shouldn't
    // have to care about what is passed through it.
  }

Indeed, just nextProps.color !== this.props.color will bail out for children changes so it is not enough. Usually people compare props shallowly so children are included in the comparison.

This begs the question: isn’t this negating the benefits of shouldComponentUpdate? If your children are static, hoisting constant elements in production would help but this doesn’t apply to cases where you rely on this.state inside the child element.

There was some discussion about taking children off of props so you might want to have a look at it for some historical context: #4694.

[benoitgrelard]

@gaearon I realise this implementation is naïve, and it does make sense when thinking about the fact that even though children are passed from the outside, they are only rendered within the parent's render function, wherever you're going to do {children}.

To be clear, this example was obviously very contrived and doesn't really make sense on its own as you probably wouldn't try to optimise things in this case.

In my real scenario, I'm however having to deal with trying to optimise things on hundreds of these components.

To give a bit more insight into what I'm building:

I am building a low level performant grid component (similar to FixedDataTable ) that can handle thousands of rows and columns, as well as handling lots of realtime updates. I am using Immutable.js for the data so that it's easier and more efficient to handle.

My problem appeared when I changed the top level Grid API from passing value getters for each cell value, (columnHeaderValueGetter, rowHeaderValueGetter and cellValueGetter) to wanting to let users render their own cells the way they want.

Using getters at the top level component meant it was easy to optimise my Cell component using shouldComponentUpdate because it would end up receiving a value prop (easy to check for changes). But this API wasn't really flexible and made it hard to implement anything else (say the user wants to have a customisable helper text on hover – using title for example, you'd have to pass all the way through another columnHeaderHelperTextGetter, etc)

From there came the idea to be able to pass your own custom components in the same fashion as FixedDataTable.

But I can't seem to be able to stop everything re-rendering even when not needed (especially for example when scrolling as setState is basically being called on requestAnimationFrame during scroll, meaning a lots of updates, for a lot of cells).

After all your comments regarding how technically difficult it would be to optimise shouldComponentUpdate for arbitrary children, I wonder what to do. And even if it was possible, it still feels ugly to me that the parent component should have to check for changes on its children's props. It feels like it breaks encapsulation and composition.

To me the whole point of being able to pass children from outside, or even as props is that you can easily compose things and separate concerns and responsibilities.

@gaearon, you mention this:

Indeed, just nextProps.color !== this.props.color will bail out for children changes so it is not enough. Usually people compare props shallowly so children are included in the comparison.

How would you do this in practice? I understand shallowCompare on the parent props (which include children) but wouldn't that only basically check for nextProps.children === this.props.children? In which case, I feel like it would be different every render...

It does feel like it negates the benefits of shouldComponentUpdate if React also offers the ability to pass arbitrary components as children or even as props.

[gaearon]

For your use case, you can take the same approach (I believe?) that FixedDataTable takes. Rather than accept children, accept renderChildren. For example,

render() {
  return (
    <MyTable
      renderRow={this.renderRow}
      renderHeader={this.renderHeader}
    />
  )
}

Make sure the function references don’t change on every render (just don’t bind them in render()) and you’ll be fine. Sure, closing over this.state won’t bring updates in this case:

renderRow() {
  // won’t update
  return <CustomRow data={this.state.data} />
}

To work around it, make any necessary data props to your table so that it can pass them back to the render* props whenever it needs:

render() {
  return (
    <MyTable
      renderRow={this.renderRow}
      renderHeader={this.renderHeader}
      data={this.state.data}
    />
  )
}

renderRow(data) {
  return <CustomRow data={data} />
}

This way, unless data changes, re-rendering <MyTable /> won’t re-render every row.

How would you do this in practice? I understand shallowCompare on the parent props (which include children) but wouldn't that only basically check for nextProps.children === this.props.children? In which case, I feel like it would be different every render...

Yes, however, most components in my experience don’t take custom children so this only comes up in limited cases, and usually shallowCompare works fine. When it doesn’t, you can make it work good by using the workaround I suggest above.

[benoitgrelard]

So you mean passing a render callback? Isn't that considered an anti-pattern?

[gaearon]

Anti-pattern is a loaded word. It’s a tool; like any tool it has upsides and downsides. In this particular case I think it’s very appropriate.

[jimfb]

Well, some anti-patterns are just bad, always. But I wouldn't classify passing a render callback as an anti-pattern, I'd just classify it as a pattern.

The pattern is used pretty commonly when a parent needs to pass necessary info to an unowned child. I think it's at least as good as cloning elements or using context.

[benoitgrelard]

What I meant was, doesn't passing a render component makes the component inherently impure? As now the render callback has access to all sorts of outside variables the function has closed on?

Meaning shouldComponentUpdate wouldn't help here?

[jimfb]

As long as the callback is pure, everything remains pure. The component is still a function of props (even though the prop happens to be a pure function).

shouldComponentUpdate can check equality of the function, to see if the function is referentially equal to the old function.

[benoitgrelard]

@gaearon I've updated the example here with the suggested technique: http://www.webpackbin.com/EJtN1ctk-

Although it works, I feel like now we've basically broken encapsulation and made Box couple to its children because it has to account for data it needs to pass through explicitly via renderChildren.

I understand that if you were to render arbitrary children in the render callback now it's somewhat practical, but considering this current example of just a string, how is it different from making the Box component accept a counter prop anyway and render it itself?

What I mean is that now the Box component is responsible for more than it should.

[gaearon]

The way I see it, you don’t get the render thousands of rows and keep perfect encapsulation 😄 . Something’s gotta give.

That said I don’t see where the encapsulation is breaking. If you rename it from counter to rowData, it’s pretty abstract and can work for different kinds of children. The fact that you pass a function rather than children themselves—well, that’s an optimization. Maybe you want to support a million rows in the future, and creating all of them is a waste anyway, so you build support for lazy creation right in your API. I don’t really see an issue with this.

[Tzook]

We came across this issue as well and we decided to solve it!
So we built an HOC that takes any component and makes its performance much better (by implementing a wiser shouldComponentUpdate)!
Take a look here: https://github.com/NoamELB/shouldComponentUpdate-Children