shouldComponentUpdate doesn't work well if component accepts children

[dantman]

React's shouldComponentUpdate based performance improvements work great for improving the performance of medium-weight components with large numbers of instances. They even work well with event handlers, as you can ignore event handler changes and instead pass a locally bound method that'll access this.props.on* on demand. However this all fails apart you start passing react elements to pure components.

'use strict';
import React, {Component, PropTypes} from 'react';
import ReactDOM from 'react-dom';
import shallowEqual from 'recompose/shallowEqual';
// shallowEqualExcluding: Fictional function that works like shallowEqual, but ignores changes to a list of props passed as the third argument

class Button extends Component {
    static propTypes = {
        icon: PropTypes.node.isRequired,
        onClick: PropTypes.func
    };

    onClick = (e) => {
        this.props.onClick(e);
    };

    shouldComponentUpdate(nextProps) {
        // @note Doesn't actually work
        return shallowEqualExcluding(this.props, nextProps, ['onClick']);
    }

    render() {
        const {icon} = this.props;

        return (
            <button onClick={this.onClick}>
                {icon}
                {/*React.cloneElement(icon, {ref: (icon) => this.iconRef = icon})*/}
            </button>
        );
    }
}

class Icon extends Component {
    static propTypes = {
        name: PropTypes.string.isRequired,
        color: PropTypes.string
    };

    shouldComponentUpdate(nextProps) {
        return shallowEqual(this.props, nextProps);
    }

    render() {
        const {name, color} = this.props;
        return getSvgIcon(name, color);
    }
}

const nilClick = () => {};
ReactDOM.render(<Button onClick={() => alert('Clicked!')} icon={<Icon name='done'} />, document.querySelector('#container'));
ReactDOM.render(<Button onClick={nilClick} icon={<Icon name='done'} />, document.querySelector('#container')); // 2nd invovation
ReactDOM.render(<Button onClick={nilClick} icon={<Icon name='cancel'} />, document.querySelector('#container')); // 3rd invovation

Given this sample; A <Button /> component that expects an icon to be passed as an icon prop and a simple <Icon />. Both are pure components and Button is also coded to not re-render whenonClick is changed. Pretend that Button actually has a heavy render() but its props and state don't change frequently.

On the second invocation, Icon should not require any prop change or render and Button should have its onClick prop changed but not require a render.
On the third invocation, Icon should require a render while Button itself does not need to render except for the change to Icon.

However in practice Button will always re-render, including during the second invocation when nothing changes.

This is because <Icon /> will always result in a new instance and will never be the same.

Normally you could work around this within the component itself, without telling users they have to store <Icon /> in a variable until they think they need to change its props; for functions you could pass a function that will use this.props.* itself and for objects you can do a deep comparison if you know the structure of the object. But for react elements, even though shouldComponentUpdate allows React to know if the current component has a render dependency on a sub-component, you do not have access to this information so Button cannot tell if Icon requires a render.

In practice this can turn out to be a problem when you're writing some libraries rather than an application. Notably Material UI suffers from this problem in production. EnhancedSwitch's render() is not light; EnhancedSwitch is used by RadioButton and Checkbox; both use a checkedIcon and uncheckedIcon React element prop; you can reasonably have 100 checkboxes on one page; even if they were pure, they cannot identify whether an icon requires an update; as a result, a render of the component containing the checkboxes to check a single checkbox will result in the render() of all 100 EnhancedSwitch instances.

I can think of a few ideas on what type of API could be added to React to solve this issue.

shouldComponentUpdate helper

The most obvious API would be a top-level React function that given the instance context, old ReactElement, and new ReactElement would return the result of a Component's shouldComponentUpdate. Then heavy parent components can use that to implement a shouldComponentUpdate that is aware of render dependencies in its children.
(As a bonus, theoretically you could temporarily remember this while you're walking the current tree; then instead of calling shouldComponentUpdate multiple times for every (potentially nested) component the result is simply that shouldComponentUpdate calls are raised up to the highest level where a component is render-dependent on them)

React.shouldComponentUpdate(this, this.props.icon, nextProps.icon)

However I expect the problem we have with this is that shouldComponentUpdate is also responsible for state dependent updates and this is supposed to be a rendered instance, not a ReactElement instance. While you know context from passing the current instance, you do not have a reference to the state from either of the props.

ref based shouldComponentUpdate helper

The second most obvious API would be a shouldComponentUpdate helper that instead uses a ref.

React.shouldComponentUpdate(this.iconRef, nextProps.icon)

The downside to this is that to get a ref for a component you didn't create, you inevitably have to use React.cloneElement.

render passthrough

The next idea I had was a render passthrough. A way during the render process for a component to say "I do not need a render()/update, but these children of mine may" which would tell React to skip render() and then run shouldComponentUpdate on the instances deeper in the tree.

However those components only know if they need updates if you pass them the new props; so a passthrough won't work. We'd instead need a way to tell react that it should not run render() but do pass on an update to a specific component instance

this.renderRef(this.iconRef, nextProps.icon);

The advantage of this over using shouldComponentUpdate is that instead of only allowing medium components wrapping light components to only render() when a child requires it; we also allow heavy components to never render() unless they themselves require it, while still allowing them to permit their light children to update.

partial renders

That shouldComponentUpdate based renderRef only applying updates to a component child feels somewhat awkward and forced though. So a more robust idea might be a partial render lifecycle that optionally runs when shouldComponentUpdate => false and can call for the render of a sub-tree that belongs to the current component.

class HeavyComponent extends Component {
    shouldComponentUpdate(nextProps) {
        // Ignore icon and children
        return nextProps.text !== this.props.text;
    }

    render() {
        const {text, icon, children} = this.props;

        text = doSomethingAbsurdlyCpuIntensiveAndHardToFactorOutOfThisComponent(text);

        return (
            <div>
                <h2>
                    {React.cloneElement(icon, {ref: (icon) => this.iconRef = icon})}
                    {text}
                </h2>
                <Wrapper ref='subtree'>
                    {children}
                </Wrapper>
        );
    }

    componentSkippedRender(nextProps/*, nextState*/) {
        this.subRender(this.iconRef, React.cloneElement(icon, {ref: (icon) => this.iconRef = icon}));

        this.subRender(
            this.refs.subtree,
            <Wrapper ref='subtree'>
                {children}
            </Wrapper>
        );
    }
}

Though this.subRender probably has potential for conflicts, so I expect the most react-line way to name that would be something like React.renderSubtreeIntoComponent(parentComponent, nextElement, component) which would be invoked using React.renderSubtreeIntoComponent(this, /* subtree */, this.refs.subtree);.

The <Wrapper> I used would be a really light component that probably would just render its children. It's there because React.renderSubtreeIntoComponent should probably not accept dom refs; this should be part of React lifecycle/walker, not part of client side browser only react-dom like ReactDOM.unstable_renderSubtreeIntoContainer.

[gaearon]

@maulerjan I don't understand what you mean by "comparing an instance". I think your conceptual model of React might be incorrect.

[satya164]

@maulerjan I think you mean component type? e.g. In the case of <Button />, the type will be Button. It could check the type and the props, but how deep will it go? Also, children's props can be dependent on what's in the render method, so there's no way to get the actual props without calling render. In the end it just ends up doing the same thing as what React's reconciler already does, just less performant.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

[stale]

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!