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
Changing shouldComponentUpdate SpecPolicy #2669
Comments
Wouldn't this require walking the entire component tree to collect all the shouldComponentUpdate return values? One of the main benefits of shouldComponentUpdate is that if you have a node with a large number of children, you can tell React that it doesn't need to talk with the hundreds/thousands of children nodes. Most components don't implement shouldComponentUpdate (it's really an emergency escape hatch for extreme cases where you need to do manual perf optimizations), and the default implementation is to return true. Also, it's very difficult to generalize this function, so it is usually used then the developer has a firm understanding of the implementation details of his/her page, and knows the behavior of all the child components. |
No, this is only assuming you've actually hit the shouldComponentUpdate during the reconciliation process to begin with. It's just changing it from only being a
Agreed, and if all data is immutable it's possible to generalize this into something that is shouldComponentUpdate: function(nextProps, nextState) {
if (typeof this.shouldComponentOverride === 'function') {
return this.shouldComponentOverride();
}
return !shallowEqual(this.props, nextProps) ||
!shallowEqual(this.state, nextState);
} The request here is to provide the inverse of generalizing into something like the var ForceUpdateMixin = {
shouldComponentUpdate: function() { return true; }
} and this would disable the behavior of the |
+1 on this |
I've had necessity for this feature in the past, so +1 on this |
just look for shouldComponentUpdate*, evaluate each and OR them together. done. |
+1 |
For what it’s worth I retract my +1 on this before. I’ve written some pretty terrible code with smart mixins and I no longer use them. Any kind of merging behavior here encourages components keeping implicit state in mixins, and this turned out to be a trouble to maintain in the app I worked on. |
Yep, I agree with @gaearon |
I wanted to propose changing
shouldComponentUpdate
from aDEFINE_ONCE
to a spec that might be termedDEFINE_MANY_ALL
.The rationale here is that sometimes you may know that it's more efficient to skip the
shouldComponentUpdate
for some reason in a component, or that a mixin may be able to define whether it should be able to skip updating based on it's own knowledge of props/state/context, but that might not hold true when combined with other components of an app.So my thought is
shouldComponentUpdate
should be changed to aDEFINE_MANY
, whereALL
of the return values must return false for the update to skip. If any return true, it will have the same behavior as the default and update.The text was updated successfully, but these errors were encountered: