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

would using shouldComponentUpdate instead of componentWillReceiveProps give performance increases? #58

Closed
faceyspacey opened this issue Oct 6, 2016 · 4 comments

Comments

@faceyspacey
Copy link
Contributor

faceyspacey commented Oct 6, 2016

For example, if transitions were triggered from shouldComponentUpdate, but it returned false, preventing any re-renderings of children, and only triggering transitions, wouldn't that increase performance greatly over using componentWillReceiveProps along with various internal states?

I know the way the component currently works relies on using its own state--so another way to ask this is: does using componentWillReceiveProps result in re-rendering of children (or additional tree parsing) even if the props passed to the children don't change? Or is React smart enough to stop not just virtual dom rendering but also parsing your react components who didn't receive any different props from parents?

It seems shouldComponentUpdate wouldn't exist if React wouldn't otherwise attempt to parse children (at least in some cases). And if that's the case, animation performance in the current implementation suffers from not making use of shouldComponentUpdate to stop propagation of updates that were solely intended to declaratively trigger animations of the Animatable parent component.

@oblador
Copy link
Owner

oblador commented Oct 14, 2016

You're onto something, but using shouldComponentUpdate here is a breaking change and might lead to unintended bugs as child components might not be pure. We could perhaps introduce it in a major version bump. For the moment it's better to make your child components implement shouldComponentUpdate/extend PureComponent if they have heavy render() functions.

@oblador
Copy link
Owner

oblador commented Nov 30, 2016

I've decided that I don't want to implement this.

  1. It doesn't solve the propagation of updates/render when triggering an animation.
  2. It's much better if this is up to the end user to decide where pure components makes sense.

@oblador oblador closed this as completed Nov 30, 2016
@faceyspacey
Copy link
Contributor Author

faceyspacey commented Nov 30, 2016

the way i ended up making my animated component is simply with a forceUpdate prop, which is compared in shouldComponentUpdate to determine whether to render children. So basically I came to the same conclusions as you, but built first-class support for it into the solution. It does have its downsides (the main one being that it's basically a requirement to always provide a hash as the forceUpdate prop), but I've found rendering on web using RNW just to be completely horrible compared to regular RN, so such things make a huge difference when animating and renderings happen at the same time.

@faceyspacey
Copy link
Contributor Author

faceyspacey commented Nov 30, 2016

for reference:

shouldComponentUpdate(nextProps, nextState) {
    let shouldUpdate = true; //non-animation-related props changed (therefore we dont need to worry about rendering during animation, and should definitely update)

    this.properties.forEach((prop) => {
      if(this.props[prop] !== nextProps[prop]) {
        shouldUpdate = false; //new props are related to animation, and therefore renderings should ideally be blocked
        this.animate(this[prop], nextProps[prop], nextProps);
      }
    });

    if(shouldUpdate === false) {
      //allow calling code to specify that they should always update during animations (i.e. because nested children change at the same time)
      if(this.props.updateOnAnimate) {
        return true;
      }

      //allow calling code to provide a hash for efficient situation-specific comparison
      if(nextProps.forceUpdate !== this.props.forceUpdate) {
        return true;
      }

      return false;
    }

    return true;
  }

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

No branches or pull requests

2 participants