Bail out if stateProps can be calculated early and did not change

[gaearon]

This should fix reduxjs/redux#1437 and #300.

If the component doesn’t care about ownProps, we can bail out of calling setState().
This PR implements this optimization.

As a reminder, we can’t do this for components that do care about ownProps due to #99.

---

Above, I said:

As a reminder, we can’t do this for components that do care about ownProps due to #99.

However in many cases it’s possible to convert a component that needs ownProps to a component that doesn’t.

How? In #279, we added the ability to specify factories instead of mapStateToProps and mapDispatchToProps. Curiously, these factories themselves do receive state and ownProps as arguments. Of course, they are only invoked once, but if you’re only using ownProps to read an ID (which is a common case in large lists) and use stable keys, you should be able to change

function mapStateToProps(state, ownProps) {
  return {
    item: state.items[ownProps.id]
  }
}

into

function makeMapStateToProps(initialState, initialOwnProps) {
  var id = initialOwnProps.id
  return function mapStateToProps(state) {
    return {
      item: state.items[id]
    }
  }
}

See? Obviously id would never get updated, but we don’t need it to (in this particular case). And we can always add ownProps later (of course at the performance cost).

I think it’s a neat trick, and it will benefit from merging this optimization, since this optimization is relevant to any mapStateToProps that doesn’t depend on ownProps.

[gaearon]

cc @slorber, @ellbee, @tgriesser who might be interested in this

[ellbee]

This one looks like a clear win to me, but I wonder if it would be worth working on a benchmark suite again like the one started in #104? I've used benchmark.js before, but not for testing UI code. Is it even really feasible?

[gaearon]

For now, I’m testing against https://github.com/mweststrate/redux-todomvc manually.
My work in progress on optimizing app itself to Redux patterns is in mweststrate/redux-todomvc#1.

[ellbee]

Yeah, I have just been reading through that issue. It is what made me start thinking about it. I might have a play around with it.

[gaearon]

👍

[gaearon]

Oh wait, I’m supposed to emoji your post inline.

[slorber]

@gaearon this is great news that connect not using component's props get faster :)

However I'm not sure the current implementation is the best we could do:

• The factory method works but is kind of unintuitive API at first
• If a connected component do care about ownProps, and these props can change (ie can't use the factory), then the component will setState even if the props don't change often

Instead of

function makeMapStateToProps(initialState, initialOwnProps) {
  var id = initialOwnProps.id
  return function mapStateToProps(state) {
    return {
      item: state.items[id]
    }
  }
}

connect(makeMapStateToProps)(Component)

I would rather have something like:

connect({
   mapperPropsSelector: ownProps => {id: ownProps.id},
   mapper: (state,{id}) => state.items[id]
})(Component)

(this is probably not the best API but you get the idea)

What it permits here is to make it clear on what the mapping function has to rely on to do its job, so you can more easily detect when props needed for the mapping have changed and run the mapper only when needed

And I think, by default, not providing any ownProps to the mapping function (ie no selector provided) would actually encourage people to write more optimized code by default (unlike factories which would be opt-in optimization), but it would be a breaking change

(This is related to my proposal for more flexible subscriptions here: #269 (comment))

[gaearon]

I’m confused: how does accepting a selector for props solve the problem of setState()? As soon as something depends on ownProps, due to #99, we are forced to delay its calls until render() at which point we have already paid the price of setState().

[ghost]

Sorry if I'm going on a tangent with this comment but I'll share some performance optimizations I implemented earlier this month when I rewrote react-redux-provide. These optimizations might not perfectly translate to react-redux but perhaps similar algorithms could be implemented.

I added a watch method to the store which accepts a reducerKey and a callback function to be called whenever the reducer returns a different state (via a reducer enhancer). The declarative nature of react-redux-provide makes it possible to automatically efficiently watch for changes to the relevant reducers, and in which case, a doUpdate flag is raised so that we can simply call forceUpdate after the action has completed, since we already know some relevant state has changed. The wrapped component's props are cached and updated accordingly per reducer. This means no need for shallow comparisons or loops, and shouldComponentUpdate can always return false (unless the component receives new props of its own, of course). And so updates should be near instantaneous as they, in theory, only require a few clock cycles per relevant component - i.e., new state -> relevant components -> update.

Condensed version:

for (let reducerKey of reducerKeys) {
  this.unsubscribe.push(
    store.watch(                  // when some reducer returns a new state
      reducerKey, nextState => {  // update wrapped component's props
        this.componentProps[reducerKey] = nextState;
        this.doUpdate = true;     // and we know for sure we should update
      }
    )
  );
}

this.unsubscribe.push(
  store.subscribe(() => {         // after action has completed
    if (this.doUpdate) {          // simply check for doUpdate flag
      this.forceUpdate();
    }
  })
);

See the full version where the same method is used for props derived from some combination of state and own props:
https://github.com/loggur/react-redux-provide/blob/master/src/provide.js#L93-L212

To elaborate a little bit more, deriving props from a combination of state and own props looks like this:

// here we'll provide some item from some list
const merge = {
  item: {           // this is run only for components with an `item` propType
    keys: ['list'], // the provided `item` depends on the state of the `list`
    get(state, props, context) {
      // this function is executed only when `list` changes
      // and components will be updated only when the `item` has changed
      const { list } = state;
      const { itemIndex } = props;

      return list[itemIndex] || null;
    }
  }
};

There are already tests to ensure renders occur only as absolutely necessary, but more in-depth performance tests are coincidentally next on my todo list. I was planning on seeing how things perform when incorporated into dbmon, but I think I'll also run some benchmarks on react-redux-provide/examples/todomvc.

[slorber]

I’m confused: how does accepting a selector for props solve the problem of setState()? As soon as something depends on ownProps, due to #99, we are forced to delay its calls until render() at which point we have already paid the price of setState().

@gaearon sorry it's hard to think well on such code late in the night :) My proposal may not prevent the setState cost finally.

The problem I see with current code is that for example if this component changes from

<Todo id={123} className="someName"/>

to

<Todo id={123} className="anotherClassName"/>

then yes we necessarily have to do a setState because ownProps have changed and we must render the child. The problem is that here our mapStateToProps is relying on ownProps but actually does only care about the id, and not className.

This line:

shouldUpdateStateProps = hasStoreStateChanged || (
            haveOwnPropsChanged && this.doStatePropsDependOnOwnProps
          )

it will make mapStateToProps be run in such a case, while it could have been avoided if we could have known that mapStateToProps is not interested to receive the className property but only the Todo id.

Also, not a big deal, but not sure this.doStatePropsDependOnOwnProps = this.finalMapStateToProps.length !== 1 is relyable if someone writes mapStateToProps with arguments[0] (yes it is unlikely... :p). It would become more relyable if that wish to depend on ownProps was more explicit

---

This is only what I can say for now, I have to study the code a bit more to say anything else :) (ie #99)

[joonhyublee]

@gaearon Here is my use case, in which early bailout yields significant performance boost.

With regards to @slorber's concern that props can change, I encountered a similar issue, and I did this hack to get around it.

Let's say we have:

//factory for map state to props, as per Dan's suggestion above
const makeMapStateToProps = (initialState, initialProps) => {
  const { userID } = initialProps; //props that's not expected to change (often)

  const mapStateToProps = (state) => {
    const userNickname = state.users[userID].nickname; //slice of state dependent on props
    const userAvatar = state.users[userID].avatar; //slice of state dependent on props

    return {
      userNickname,
      userAvatar
    };
  };

  return mapStateToProps;
}

const UserComponent = React.createClass({
  // component implementation
});

export default connect(makeMapStateToProps)(UserComponent);

Of course, userID isn't expected to change, but there may be some cases where it does. For example, the above component may be a react-router route component, which gets its userID from the pathname: http://example.com/userpage/cat. When the path changes to http://example.com/userpage/dog the component holds on to stale userID (cat) and this causes problems. (because react router doesn't re-mount for the same route)

I could revert back to mapStateToProps (state, ownProps), but I would loose the performance boost gained from the early bailout. So instead, I do this:

//instead of this,
export default connect(makeMapStateToProps)(UserComponent);

//do this:
const ConnectedComponent = connect(makeMapStateToProps)(UserComponent);
export default (props) => (
  <ConnectedComponent key={ props.userID } {...props}/>
);

Because of key, when userID is changed, the old component is completely unmounted, and new one is mounted instead, with the updated props. It's a bit hacky, but I think this is an acceptable trade-off. I guess for multiple props that aren't expected to change often I can stringify as a key:

export default (props) => (
  <ConnectedComponent key={ props.userID + '_' + props.anotherSparselyChangedProp }/>
);

but that .. is beyond ugly. It does bother me there are so many different steps within different levels involved in simply invalidating and updating a component, but was the only way I could make this work.

[slorber]

nice trick @joonhyublee :D

Maybe this usecase will be easier to handle once the code of connect becomes much simpler, and it might with #368