areMergedPropsEqual doesn't seem to run

[bdwain]

I have a connected component near the root of my app that looks like this

//render method for component
render(){
  return this.props.children;
}

function mapStateToProps(state){
  return {
    state //used in some context methods that the component has which don't care about rendering
  };
}

function areMergedPropsEqual(next, prev){
  console.log('hi');
  return next.children === prev.children;
}

export default connect(mapStateToProps, undefined, undefined, {areMergedPropsEqual})(Component);

i added the areMergedPropsEqual option because i was worried every redux change would trigger a rerender of the entire app unnecessarily.

The problem is that areMergedPropsEqual seems to be ignored. I put a breakpoint in there and it never gets hit. and there's no console logs.

Is it possible that the option is being ignored?

[bdwain]

Also, after thinking about alternatives, can't you just use shouldComponentUpdate in the component being wrapped to function the same as areMergedPropsEqual? It worked to prevent the extra rerenders and I can't really see what areMergedPropsEqual provides that you can't get with shouldComponentUpdate. Except maybe avoiding an expensive componentWillReceiveProps.

[markerikson]

One of the overall goals of the v5 rewrite is to do as much work as possible in selectors, and only force updates in a component once it's confirmed that the data has really changed (as opposed to v4 and earlier, which used React's setState() to queue up update checks after a store notification, and actually did the real checking in the wrapper component's render() method.)

Two thoughts on this issue:

First, the code definitely starts by using a default shallowEqual function as the areMergedPropsEqual implementation, but it looks like it should use a function you provide if given. Not sure why it wouldn't be getting called in your case.

That said, why are you actually trying to supply your own function in the first place? I'm kinda confused by your example in a couple ways. The point of a mergeProps comparison is to check the output of combined results from mapState and mapDispatch, not to actually look at the children prop passed in by React. I also think you may be overly concerned about perf and re-rendering. Are you having actual performance problems in your app? Have you done any benchmarks or measuring to see what parts of the code may be bottlenecks, if any?

[bdwain]

Hi. I'm not totally sure that the optimization is necessary. But my project often runs on webviews in cheap slow tablets, and when it's slow, it's really slow. Either way though, this looked like a bug, so I wanted to bring it to your attention.

I was mostly just worried about putting the entire state object in the results from mapStateToProps, which seemed like it would trigger a rerender of my component (which is at the root of the app) every state update, and that that would trigger rerenders of every component below it, which had the potential to be slow. This just seemed like it would be a really easy way to prevent that from being an issue in the first place, and with little risk.

[markerikson]

If you're worried about that, the recommended approach is to connect more components. That way, each of them rely on more granular pieces of the store state, and will need to re-render less often. See http://redux.js.org/docs/faq/ReactRedux.html#react-multiple-components and http://redux.js.org/docs/faq/Performance.html#performance-scaling for some more info on the topic.

[bdwain]

Yea I am doing that for the most part. I think with what i'm doing, removing this component and connecting every component that used this would just be a lot of extra boilerplate because lots of components will use the method this component provides (it's an analytics method).

So I was looking for a way to provide the context method that exposed the state without triggering so many updates if possible. areMergedPropsEqual was probably a weird way of doing that. shouldComponentUpdate seems to work, even though i agree that normally this would just make more sense to connect the child components.

[bdwain]

Also thanks for your help @markerikson. If you want I can try to come up with a gist that has a reproduction of the areMergedPropsEqual method not being called. Though all of the key info is in my snippet in the original comment.

[markerikson]

Depending on how the analytics piece works: would it make more sense to actually put it in React context, or create a Redux middleware that uses it?

[jimbolla]

There are optimizations that will avoid the call when it knows it can. I'm on mobile otherwise I'd point them out

[bdwain]

The analytics piece is on the react conext. Here's the method exposed on context.

  sendAnalytics(dataType, data){
    window.analytics.send(new dataType(this.props.state, data))
  }

basically there are a lot of "dataTypes" which are just functions that format the data you pass and pull in other data from the state. Which data they pull in from the state is up to the individual data type, so in order to do this generically it seemed the only way was to pass the whole state object to the datatype.

[bdwain]

and if i was to connect every child component instead of the one root component, all of the children components would still need to get the entire state object from mapStateToProps to pass to dataType, or they would have to know what pieces of data the dataType pulled from the state, and that would mean repeating that everywhere an individual dataType was used.

[bdwain]

@jimbolla won't shouldComponentUpdate do that? It seems to work in my specific case at least.

[vevo-var-nazari]

I'm running into the same issue. I'm passing in an options object with pure:true as well one of the allowed functions that simply logs to console. The console log never runs

const areStatesEqual = ()=>{
    console.log("--testing")
    return true;
}

export default connect((store, props) => {
    return {
        thing: thing,
        someRef: someRef
    }
}, ConnectRedux.mapDispatchToProps, null, { pure: true, areStatesEqual })(ConnectRedux.safeImmutableWrapper(Component))

[jimbolla]

areMergedPropsEqual is never called if you use the default for mergeProps because the answer will always be false in that case. Since mergeProps is only called if stateProps, dispatchProps, or ownProps has changes, the default mergeProps will always produce an object that has different values. Avoiding that extra shallow compare is a significant perf boost.

Should probably update the API docs to clarify this.

[TomMahle]

I'm seeing a case where ownProps are certainly changing, stateProps are not referenceEqual, but areMergedPropsEqual still does not run.

It seems from some quick testing that passing null or undefined as mergeProps causes areMergedPropsEqual not to run. Replacing the falsy mergeProps value with the default implementation

function defaultMergeProps(stateProps, dispatchProps, ownProps) { return { ...ownProps, ...stateProps, ...dispatchProps } }

seems to cause areMergedPropsEqual to run as expected and described in @jimbolla 's comment.