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

Changing shouldComponentUpdate SpecPolicy #2669

Closed
tgriesser opened this issue Dec 6, 2014 · 8 comments
Closed

Changing shouldComponentUpdate SpecPolicy #2669

tgriesser opened this issue Dec 6, 2014 · 8 comments

Comments

@tgriesser
Copy link

I wanted to propose changing shouldComponentUpdate from a DEFINE_ONCE to a spec that might be termed DEFINE_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 a DEFINE_MANY, where ALL 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.

@jimfb
Copy link
Contributor

jimfb commented Dec 7, 2014

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.

@jimfb jimfb closed this as completed Dec 7, 2014
@jimfb jimfb reopened this Dec 7, 2014
@tgriesser
Copy link
Author

Wouldn't this require walking the entire component tree to collect all the shouldComponentUpdate return values?

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 DEFINE_ONE to something similar to a DEFINE_MANY, but with the behavior of Array.prototype.some. If any of them are true, they're all true.

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.

Agreed, and if all data is immutable it's possible to generalize this into something that is injectMixin'ed at the top-level (as is the behavior in something like om). In our application, that's the case most of the time via immutable-js, and it makes the performance insanely fast with hundreds / thousands of components all rendering from the top. There have been a few cases where we'd like to "escape the escape hatch" so to speak. Currently the workaround is to do something like:

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 PureRenderMixin, except with a cleaner approach than requiring a forceUpdate. Sometimes you just know you always want a re-render, and you could just do a mixin with:

var ForceUpdateMixin = {
  shouldComponentUpdate: function() { return true; }
}

and this would disable the behavior of the PureRenderMixin on an component-by-component basis.

@gaearon
Copy link
Collaborator

gaearon commented Dec 11, 2014

+1 on this

@JohnyDays
Copy link

I've had necessity for this feature in the past, so +1 on this

@rektide
Copy link

rektide commented Mar 21, 2015

just look for shouldComponentUpdate*, evaluate each and OR them together. done.

@Tvaroh
Copy link

Tvaroh commented Jun 24, 2015

+1

@gaearon
Copy link
Collaborator

gaearon commented Mar 18, 2016

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.

@tgriesser
Copy link
Author

Yep, I agree with @gaearon

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

7 participants