Only re-render when you need to (Potentially huge perf boost)

[JAStanton]

What would you guys say to adding something like this:

shouldComponentUpdate(nextProps, nextState) {
  return !_.isEqual(this.props, nextProps) || !_.isEqual(this.state, nextState);
}

which would block calling render if the props or state haven't changed. If added to components that have simple props (i.e. don't take React elements) and have simple state but have heavy render methods (like TextField)?

I see this as a potentially huge perf boost when you have a giant form with lots of TextField and you're using valueLink. That is, every keystroke of one text field causes all sibling text field's to change to do the state changing.

Thoughts?

[oliviertassinari]

To add to the discussion: facebook/react#2517.

[nevir]

Hmm. Is it common to change muiTheme after a component has been instantiated? It seems like that should be static a vast majority of the time - and that this would be a good simplifying assumption.

If it is common though; maybe make this an opt in? muiTheme.static = true or something?

[oliviertassinari]

@nevir This can be a good tradeoff

[hai-cea]

Thanks @JAStanton - There also a discussion about this same issue in #1040

I agree with @oliviertassinari that @nevir's idea seems like a good tradeoff.

[nevir]

(Or maybe opt out, actually, if you think a static theme is the norm)

[oliviertassinari]

@hai-cea I believe that this is a more general issue than #1040.
We can try to use a custom PureRenderMixins that take into account a static boolean with a default value false.
Basically, if static is false, then the shouldComponentUpdate should return true. Otherwise be using the shallowEqual.
I believe this mixing can be applied to every component without breaking too much things. Anyone wants to try?

[cgestes]

Till this issue with context is not fixed upstream, the PureRenderMixins should be disactivable as ones can write a component that want to rerender based on the context. (using react-router components for example).

I agree that it would be nice to optimize material-ui with PureRenderMixin. I guess there is also some work to do around focus/touch ripple, they generate a lot of useless rerender at the moment.

Speaking of something we can see, scrolling a list on a Phone is not really smooth right now. (should not generate rerender or very small one)

[hai-cea]

@cgestes I changed focus ripples to only render when the actual component has keyboard focus - d303cac. Hopefully that will help some.

I know there are some components that do not require theme variables. I'm thinking that it would be safe for us to optimize those components? I may not be understanding some special requirements of react router that you've mentioned.

[hai-cea]

I've been giving this much more thought since I'm in the process of optimizing menus. I think that we should be able to handle context changes pretty easily. It turns out that each component only cares about handful of theme context variables. Which means, each component can track changes to those variables and decide whether or not shouldComponentUpdate should return true or false. For those components that do not use theme variables, we can safely use PureRenderMixin.

This can all be handled in a new mixin: ContextPureRenderMixin. Here's how it would work:

const MyComponent = React.createClass({

  contextTypes: {
    muiTheme: React.PropTypes.object,
  },

  mixins: [ContextPureRenderMixin],

  ...

  getContextVars() {
    return {
      fontFamily: this.context.muiTheme.contentFontFamily,
    };
  }
});

And here's the implmentation for the mixin:

module.exports = {

  //Save off context vars for future compares in shouldComponentUpdate
  componentDidMount() {
    this._lastContextVars = this.getContextVars();
  },

  //Don't update if state, prop, and context are equal
  shouldComponentUpdate(nextProps, nextState) {

    const currentContextVars = this.getContextVars();
    const contextChanged = !shallowEqual(this._lastContextVars, currentContextVars);

    //Save off context vars for future compares in shouldComponentUpdate
    if (contextChanged) this._lastContextVars = currentContextVars;

    return (
      !shallowEqual(this.props, nextProps) ||
      !shallowEqual(this.state, nextState) ||
      contextChanged
    );
  },

};

What do you guys think of this approach? @JAStanton @oliviertassinari @nevir @cgestes

[JAStanton]

Yeah sounds like you get the best of both worlds!

[nevir]

👍

[cgestes]

What about changes in context only used by childrens (wether mui component
or not)?

On Fri, Jul 31, 2015, 06:52 Ian MacLeod notifications@github.com wrote:

[image: 👍]

—
Reply to this email directly or view it on GitHub
#1176 (comment)
.

[hai-cea]

@cgestes The mixin would be added on the children level. So if a child uses context variables to affect its render, then it will add this new mixin if it's pure. Is that what you're asking?

[cgestes]

In the current context impl of react (0.13) Iam not sure that child are
rerender if needed when a parent use a purerendermixin. (they did some
change in the new beta in regard to that)

On Fri, Jul 31, 2015, 13:56 Hai Nguyen notifications@github.com wrote:

@cgestes https://github.com/cgestes The mixin would be added on the
children level. So if a child uses context variables to affect its render,
then it will add this new mixin if it's pure. Is that what you're asking?

—
Reply to this email directly or view it on GitHub
#1176 (comment)
.

[hai-cea]

@cgestes The change that I'm proposing would be to implement a new mixin that would account for this.

[hai-cea]

@JAStanton @nevir @cgestes I created a PR to address this issue. Hope it makes sense. Please let me know what you think. #1397

[oliviertassinari]

Following the change @gaearon proposed here facebook/react#2517 and the issue he raised:

Yeah, the problem is middle components don't know what context some distant child might need.

Hopefully, Material-ui components are very often leaves of the react tree.
I think that we could highly reduce the cost of adding the ContextPure to all of our components by assuming that the muiTheme context is immutable. Hence we could remove the relevantContextKeysEqual, getRelevantContextKeys and getChildrenClasses methods.

We would still have one issue in the case where a material-ui component is not a leaf and the underlying component depend on the context but relevantContextKeysEqual and the current version don't solve this anyway.
For instance, if only the muiTheme context change:

<Paper> // Rerender
  <MyCustomPureRenderComponent> // Don't rerender
    <Button /> // Don't rerender
  </MyCustomPureRenderComponent>
</Paper>

Maybe the solution is to provide an HOC that people have to apply to MyCustomPureRenderComponent in order to make it rerender.
But since it's already an issue with the master branch, I think that we can ignore it for now and implement a pure rendering feature.

[shaurya947]

@oliviertassinari if you see the React doc page on context, it seems like the issue you mentioned (and all surrounding issues -- it was quite hard to track them sequentially) resolved to this:

By adding childContextTypes and getChildContext to MessageList (the context provider), React passes the information down automatically and any component in the subtree (in this case, Button) can access it by defining contextTypes.

which means that context is accessible to all children in the subtree, and they can opt in as they like. With that being said, we do have most of the things mentioned above implemented today (context-pure mixin, the statics property with all the necessary functions etc.).

The recommended way of dealing with muiTheme is to make it immutable. Therefore, whenever any property of the rawTheme changes, a new muiTheme object is computed -- definite immutability. However, If you look at the example here (scroll down to "Overriding Theme Variables"), whenever you modify the muiTheme object at some level in the subtree, only components descending from it are affected -- which is perhaps some mechanism of immutable behavior.

I think this can be closed now.