Skip to content
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

Optimizing componentWillReceiveProps and shouldComponentUpdate #5650

Closed
baaygun opened this issue Dec 11, 2015 · 5 comments
Closed

Optimizing componentWillReceiveProps and shouldComponentUpdate #5650

baaygun opened this issue Dec 11, 2015 · 5 comments

Comments

@baaygun
Copy link

baaygun commented Dec 11, 2015

React component lifecycle dictates that when new props roll in, componentWillReceiveProps gets called, where you can calculate the new state and set it without a second cycle. Afterwards, shouldComponentUpdate gets called and we can compare props and state of last cycle with the next one.

However, if state is a product of the props, it makes more sense to check whether the props have changed before calculating the state, so unnecessary calculations can be avoided. This produces some ugly looking code where componentWillReceiveProps first runs the code that would otherwise be in shouldComponentUpdate and based on that result sets a new state, or terminates.

Simplified Example:

componentWillReceiveProps: function (nextProps) {
    if (nextProps === this.props) {
        return;
    }

    // Calculate new state
    this.setState(...);
},

shouldComponentUpdate: function (nextProps, nextState) {
    return this.state !== nextState;
}

Is there a cleaner way of approaching this where no resources are wasted on unnecessary state calculations?

@milesj
Copy link
Contributor

milesj commented Jan 6, 2016

Correct me if I am wrong, but it currently works this way. The componentWillReceiveProps method only triggers if the props have changed, as seen here: https://github.com/facebook/react/blob/master/src/renderers/shared/reconciler/ReactCompositeComponent.js#L668

This line seems unnecessary:

    if (nextProps === this.props) {
        return;
    }

As you can see by the following lines, the nextState is generated and then passed to shouldComponentUpdate, which is still required, as either state/props/context may have changed. However, there could be some logic that does an additional check on whether to call shouldComponentUpdate.

I could test this in my current pull request here: #5787

@jimfb
Copy link
Contributor

jimfb commented Jan 6, 2016

@milesj That is incorrect. componentWillReceiveProps does not guarantee that props have changed, as I have already mentioned to you on your diff in #5776 (comment)

@Volfied Your approach seems pretty reasonable to me.

Background: I understand that it's a little bit of boilerplate, but if the order of the two lifecycles were switched, users would have the opposite problem where you are calculating the new state in shouldComponentUpdate because you need to know if the state changed in order to decide if you were going to update. Even worse, shouldComponentUpdate might return false, but the component might still need to know about the new props and save them into internal state. Therefore, the order couldn't be switched.

Your original question was:

Is there a cleaner way of approaching this where no resources are wasted on unnecessary state calculations?

I don't know of a cleaner way off hand, but perhaps someone else will have ideas. Your solution seemed fine to me. On a tangential note: Keep in mind that ideally your state should not be a function of your props (you should try to keep them separate and independent). For more info on keeping props+state separate: https://facebook.github.io/react/tips/props-in-getInitialState-as-anti-pattern.html Also, lifecycle methods are intended to be an escape hatch, rather than a recurring pattern in your code.

Your question is a usage question, rather than a bug in the React core. Usage questions are better answered on sites like StackOverflow, as we try to use github issues for tracking bugs in the React core. For this reason, the issue was closed. You're welcomed to continue the conversation here, or move it to a site intended for these sort of questions (like StackOverflow).

Anyway, hopefully you find the above info useful. Happy coding!

@milesj
Copy link
Contributor

milesj commented Jan 6, 2016

Right, right, but is there a specific reason that componentWillReceiveProps is called even though nothing changed? That seems rather odd.

On a related note, the docs should probably be changed, as "Invoked when a component is receiving new props." is a bit misleading.

@benjycui
Copy link
Contributor

benjycui commented Jan 6, 2016

Right, right, but is there a specific reason that componentWillReceiveProps is called even though nothing changed? That seems rather odd.

It is expensive to deep compare props every time. So, the easiest way is to call componentWillReceiveProps every time.

@jimfb
Copy link
Contributor

jimfb commented Jan 6, 2016

#5790

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

No branches or pull requests

4 participants