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

Children prop gets recreated killing PureComponent optimizations #8669

Closed
davegri opened this Issue Jan 1, 2017 · 15 comments

Comments

Projects
None yet
5 participants
@davegri

davegri commented Jan 1, 2017

Do you want to request a feature or report a bug?

Report a possible bug

What is the current behavior?

When Component A renders Component B with a children prop that is a react component / JSX fragment any render of component A will recreate said children prop

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://jsfiddle.net or similar (template: https://jsfiddle.net/reactjs/69z2wepo/).

Example Code:

import  React from 'react';

class ComponentWithChildren extends React.PureComponent {
  render() {
    console.log("RENDER COMP")
    return <span>Hello{this.props.children}</span>
  }
}

class Children extends React.PureComponent {
  render() {
    return (
      <div>
        these are children
        <span>nested</span>
      </div>
    )
  }
}

class App extends React.PureComponent {

  update = () => this.setState({ count: this.state.count + 1 })

  state = {
    count: 0
  }

  render() {
    console.log("RENDER APP")
    return (
      <div>
        <button onClick={this.update}>Update</button>
        <ComponentWithChildren>
          <Children />
        </ComponentWithChildren>
      </div>
    )
  }
}

export default App;

What is the expected behavior?

I would expect ComponentWithChildren not to re-render because none of its props actually changed

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

React 15.4.1

@acdlite

This comment has been minimized.

Show comment
Hide comment
@acdlite

acdlite Jan 2, 2017

Member

This is the expected behavior.

The reason ComponentWithChildren re-renders every time is because <Children /> === <Children /> always evaluates to false. This may seem unintuitive, but it's because <Children /> is sugar for React.createElement(Children), which returns a new object every time it is called.

To fix, you can cache the <Children /> element to ensure that the value is preserved across renders:

class App extends React.PureComponent {

  update = () => this.setState({ count: this.state.count + 1 })

  state = {
    count: 0
  }

  children = <Children />;

  render() {
    console.log("RENDER APP")
    return (
      <div>
        <button onClick={this.update}>Update</button>
        <ComponentWithChildren>
          {this.children}
        </ComponentWithChildren>
      </div>
    )
  }
}
Member

acdlite commented Jan 2, 2017

This is the expected behavior.

The reason ComponentWithChildren re-renders every time is because <Children /> === <Children /> always evaluates to false. This may seem unintuitive, but it's because <Children /> is sugar for React.createElement(Children), which returns a new object every time it is called.

To fix, you can cache the <Children /> element to ensure that the value is preserved across renders:

class App extends React.PureComponent {

  update = () => this.setState({ count: this.state.count + 1 })

  state = {
    count: 0
  }

  children = <Children />;

  render() {
    console.log("RENDER APP")
    return (
      <div>
        <button onClick={this.update}>Update</button>
        <ComponentWithChildren>
          {this.children}
        </ComponentWithChildren>
      </div>
    )
  }
}

@acdlite acdlite closed this Jan 2, 2017

@davegri

This comment has been minimized.

Show comment
Hide comment
@davegri

davegri Jan 3, 2017

Hey, thanks for the answer.
what would be the appropriate solution if <Children /> needed to receive props?

davegri commented Jan 3, 2017

Hey, thanks for the answer.
what would be the appropriate solution if <Children /> needed to receive props?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 3, 2017

Member

If props are the same every time, you can hoist <Children prop={myProp} /> to a constant, just like in the example above.

If it receives different props every time then it’s not “the same” on updates. Therefore it makes sense for PureComponent to not skip rendering since otherwise you won’t see any changes below. However you are free to write a Component with a custom shouldComponentUpdate implementation if you want to use even more granular heuristics to determine whether to update.

Member

gaearon commented Jan 3, 2017

If props are the same every time, you can hoist <Children prop={myProp} /> to a constant, just like in the example above.

If it receives different props every time then it’s not “the same” on updates. Therefore it makes sense for PureComponent to not skip rendering since otherwise you won’t see any changes below. However you are free to write a Component with a custom shouldComponentUpdate implementation if you want to use even more granular heuristics to determine whether to update.

@davegri

This comment has been minimized.

Show comment
Hide comment
@davegri

davegri Jan 3, 2017

@gaearon thanks for chiming in, were having a lot of trouble with optimizing these cases. One approach we have taken is passing a renderChildren function instead of passing children at all. However children is a very readable syntax and I don't want to be too quick throwing out the baby with the bathwater.

The problematic case is mainly when you have children that depend on props which are dynamic but that doesn't mean they always change, I don't want the children to be recreated when the props don't change.

I know this sounds a bit like overkill but out goal currently is not to have a single unnecessary render (render being just entering the render function)

davegri commented Jan 3, 2017

@gaearon thanks for chiming in, were having a lot of trouble with optimizing these cases. One approach we have taken is passing a renderChildren function instead of passing children at all. However children is a very readable syntax and I don't want to be too quick throwing out the baby with the bathwater.

The problematic case is mainly when you have children that depend on props which are dynamic but that doesn't mean they always change, I don't want the children to be recreated when the props don't change.

I know this sounds a bit like overkill but out goal currently is not to have a single unnecessary render (render being just entering the render function)

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 3, 2017

Member

@davegri

You may be optimizing for the wrong thing. Running render() itself is not that expensive. The expensive part is recursively updating a large tree very often. If you can “cut off” fast updates at reasonable depth it’s fine for some components to always render.

It’s hard to offer more help without a specific example you’re struggling with.

Member

gaearon commented Jan 3, 2017

@davegri

You may be optimizing for the wrong thing. Running render() itself is not that expensive. The expensive part is recursively updating a large tree very often. If you can “cut off” fast updates at reasonable depth it’s fine for some components to always render.

It’s hard to offer more help without a specific example you’re struggling with.

@davegri

This comment has been minimized.

Show comment
Hide comment
@davegri

davegri Jan 3, 2017

@gaearon

I agree with you that our optimizations may be extreme, but assuming that we have decided we want this optimization. and we have code like:

<ComponentWithChildren>
  <span> this.props.text </span>
</ComponentWithChildren>

we don't want ComponentWithChildren's children prop to always change and cause a re-render, but only be recreated when this.props.text changes

as I said. one solution was

// on class
renderChildren = () => <span> this.props.text </span>

// in render
<ComponentWithChildren renderChildren={this.renderChildren} />

Like I said however, I dislike this approach because its much less readable.

I was wondering if you have any other solutions or suggestions?

davegri commented Jan 3, 2017

@gaearon

I agree with you that our optimizations may be extreme, but assuming that we have decided we want this optimization. and we have code like:

<ComponentWithChildren>
  <span> this.props.text </span>
</ComponentWithChildren>

we don't want ComponentWithChildren's children prop to always change and cause a re-render, but only be recreated when this.props.text changes

as I said. one solution was

// on class
renderChildren = () => <span> this.props.text </span>

// in render
<ComponentWithChildren renderChildren={this.renderChildren} />

Like I said however, I dislike this approach because its much less readable.

I was wondering if you have any other solutions or suggestions?

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 3, 2017

Member

we don't want ComponentWithChildren's children prop to always change and cause a re-render, but only be recreated when this.props.text changes

If you just pass

<ComponentWithChildren>
  {this.props.text}
</ComponentWithChildren>

(with no span), that's exactly what would happen.

I don’t think the approach you suggested works:

// on class
renderChildren = () => <span> this.props.text </span>

// in render
<ComponentWithChildren renderChildren={this.renderChildren} />

In this case even if this.props.text changes, ComponentWithChildren doesn’t know, and forgets to re-render. It would re-render if you also happen to pass text itself as a prop:

<ComponentWithChildren renderChildren={this.renderChildren} text={this.props.text} />

But at this point you’re just duplicating information, and you might as well create <SpecificComponentWithChildren text={this.props.text} /> that is also a PureComponent and that renders <ComponentWithChildren><span>{this.props.text}</span></ComponentWithChildren>.

Member

gaearon commented Jan 3, 2017

we don't want ComponentWithChildren's children prop to always change and cause a re-render, but only be recreated when this.props.text changes

If you just pass

<ComponentWithChildren>
  {this.props.text}
</ComponentWithChildren>

(with no span), that's exactly what would happen.

I don’t think the approach you suggested works:

// on class
renderChildren = () => <span> this.props.text </span>

// in render
<ComponentWithChildren renderChildren={this.renderChildren} />

In this case even if this.props.text changes, ComponentWithChildren doesn’t know, and forgets to re-render. It would re-render if you also happen to pass text itself as a prop:

<ComponentWithChildren renderChildren={this.renderChildren} text={this.props.text} />

But at this point you’re just duplicating information, and you might as well create <SpecificComponentWithChildren text={this.props.text} /> that is also a PureComponent and that renders <ComponentWithChildren><span>{this.props.text}</span></ComponentWithChildren>.

@davegri

This comment has been minimized.

Show comment
Hide comment
@davegri

davegri Jan 3, 2017

@gaearon

Your right about the function solution not working, thanks for that

That example was simple but the point is that I may have a more complex structure that can't be represented with a primitive. Is there no good way of solving this?

a real scenario from our code:

<SmallContactContainer
  userId={status === 'Reassigned' ? originalOwnerId : ownerId}
  retrieveQueue
  renderChildren={this.renderSmallContactContent}
>
  <div className={classes.dataItem}>
    {this.getStatusText()}
    {this.renderNewOwnerForReassigned()}
  </div>
</SmallContactContainer>

davegri commented Jan 3, 2017

@gaearon

Your right about the function solution not working, thanks for that

That example was simple but the point is that I may have a more complex structure that can't be represented with a primitive. Is there no good way of solving this?

a real scenario from our code:

<SmallContactContainer
  userId={status === 'Reassigned' ? originalOwnerId : ownerId}
  retrieveQueue
  renderChildren={this.renderSmallContactContent}
>
  <div className={classes.dataItem}>
    {this.getStatusText()}
    {this.renderNewOwnerForReassigned()}
  </div>
</SmallContactContainer>
@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jan 3, 2017

Member

Does what I suggested above in the last paragraph work for you? Just create a new component that encapsulates this render function, and make it a PureComponent.

Member

gaearon commented Jan 3, 2017

Does what I suggested above in the last paragraph work for you? Just create a new component that encapsulates this render function, and make it a PureComponent.

@davegri

This comment has been minimized.

Show comment
Hide comment
@davegri

davegri Jan 3, 2017

I thought you were saying to encapsulate the children into a component, but yeah I guess thats a good solution, thanks :)

davegri commented Jan 3, 2017

I thought you were saying to encapsulate the children into a component, but yeah I guess thats a good solution, thanks :)

@ackvf

This comment has been minimized.

Show comment
Hide comment
@ackvf

ackvf Sep 4, 2017

@acdlite As you wrote, <Children/> === <Children/> evaluates to false as it's actually a new instance of Children and the render of parent component is called, but what happens with the <Children/> component's mounting? It doesn't get remounted, right?

ackvf commented Sep 4, 2017

@acdlite As you wrote, <Children/> === <Children/> evaluates to false as it's actually a new instance of Children and the render of parent component is called, but what happens with the <Children/> component's mounting? It doesn't get remounted, right?

@AlexGalays

This comment has been minimized.

Show comment
Hide comment
@AlexGalays

AlexGalays Mar 16, 2018

@ackvf

The instance reference changed but its type (and key) is still the same, so the underlying DOM Element needs no update and the same component remains mounted.

AlexGalays commented Mar 16, 2018

@ackvf

The instance reference changed but its type (and key) is still the same, so the underlying DOM Element needs no update and the same component remains mounted.

@ackvf

This comment has been minimized.

Show comment
Hide comment
@ackvf

ackvf Mar 17, 2018

So, the instance reference changes, but not a single lifecycle method is called on the new object, not even the constructor. What am I missing? (I haven't gone through the code yet - I find that difficult to start with.)

ackvf commented Mar 17, 2018

So, the instance reference changes, but not a single lifecycle method is called on the new object, not even the constructor. What am I missing? (I haven't gone through the code yet - I find that difficult to start with.)

@AlexGalays

This comment has been minimized.

Show comment
Hide comment
@AlexGalays

AlexGalays Mar 17, 2018

@ackvf Not even the constructor, no. Since react works with lightweight nodes that just points to which function/tag/class should be invoked/newed... SHOULD the diffing decide it's a new element.

AlexGalays commented Mar 17, 2018

@ackvf Not even the constructor, no. Since react works with lightweight nodes that just points to which function/tag/class should be invoked/newed... SHOULD the diffing decide it's a new element.

@ackvf

This comment has been minimized.

Show comment
Hide comment
@ackvf

ackvf Mar 26, 2018

So when <Children/> !== <Children/>, but the output is same, it means that new Element instance is used (as in React.createElement(...), but both instances internally point to same class instance so that no lifecycle methods or constructor need to be called?

In other words, when pseudo-transpiled:
React.createElement(Children, ...) !== React.createElement(Children, ...) where Children === Children ?

Then if my understanding is correct, both functions refer to the class Children object and only internally they may call the new operator.

ackvf commented Mar 26, 2018

So when <Children/> !== <Children/>, but the output is same, it means that new Element instance is used (as in React.createElement(...), but both instances internally point to same class instance so that no lifecycle methods or constructor need to be called?

In other words, when pseudo-transpiled:
React.createElement(Children, ...) !== React.createElement(Children, ...) where Children === Children ?

Then if my understanding is correct, both functions refer to the class Children object and only internally they may call the new operator.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment