Skip to content

Conversation

@sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Oct 21, 2016

Builds on #8015

This fix relies on the props and state objects being different to know if we can avoid a componentDidUpdate. This is not a great solution because we actually want to use the new props/state object even if sCU returns false. That's the current semantics and it can actually be important because new parent rerenders that are able to reuse props objects are more likely to have the new props object so we won't be able to quickly bail out next time. For example:

class Foo extends React.Component {
  state = { tick: 0 };
  componentDidMount() {
    setInterval(() => this.setState({ tick: this.state.tick + 1 }));
  }
  render() {
     return this.props.children;
  }
}

I don't have a better idea atm though.

@sebmarkbage sebmarkbage force-pushed the nodidupdateforbailout branch from 82a616f to 2b3b7c6 Compare October 21, 2016 19:00
@sebmarkbage sebmarkbage changed the title Don't call componentDidUpdate if shouldComponentUpdate returns false [Fiber] Don't call componentDidUpdate if shouldComponentUpdate returns false Oct 25, 2016
expect(ops).toEqual([
'componentWillReceiveProps',
'shouldComponentUpdate',
// no componenWillUpdate
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo

This fix relies on the props and state objects being different
to know if we can avoid a componentDidUpdate. This is not a great
solution because we actually want to use the new props/state
object even if sCU returns false. That's the current semantics
and it can actually be important because new rerenders that are
able to reuse props objects are more likely to have the new props
object so we won't be able to quickly bail out next time.
I don't have a better idea atm though.
@sebmarkbage sebmarkbage force-pushed the nodidupdateforbailout branch from 2b3b7c6 to 46bde58 Compare October 25, 2016 22:21
@sebmarkbage sebmarkbage merged commit 51e1937 into facebook:master Oct 25, 2016
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

Successfully merging this pull request may close these issues.

3 participants