Rule proposal: no impure mapStateToProps

[lencioni]

Since connected components are PureComponents, it is important for mapStateToProps to avoid creating new objects with identical data every time. If this happens, it will cause unnecessary re-renders, leading to bad performance.

Writing a rule to catch all of these scenarios might be too difficult, but there is probably a subset of scenarios that could be trivially linted against. Consider the following mapStateToProps:

function mapStateToProps(state) {
  return {
    foo: {
      bar: state.foobar,
    },
  };
}

What do you think about adding a rule that flags a few of these simple cases?

[DianaSuvorova]

@lencioni, can you elaborate? What the correct mapStateToProps would look like for this use case? Do we want to enforce that the props is a flat object?

[lencioni]

A flat object would be okay if the components work that way. But, this is also a problem for arrays, e.g. this will always cause re-renders:

function mapStateTopProps(state) {
  return {
    foo: [],
  };
}

In this case, if it is always an empty array, you could hoist the definition of the empty array to the module level, so it is always referentially equivalent.

Another option is to use something like reselect.

I think the purpose of this rule would not be to enforce a flat object, but just to point out the unnecessary creation of new objects that cause re-renders. The documentation can point to some possible solutions, based on the specific situation.

[DianaSuvorova]

i see, this makes sense, let me generate the list of test cases first and run it by you.
Also the no-impure doesn't really sound to me descriptive of what this rule is trying to do. I don't have better suggestions though at the moment.

[lencioni]

Yeah, agreed on the naming. Maybe there's some terminology from react-redux we can use?

[DianaSuvorova]

I think It might make sense to create 2 new rules as per your suggestion instead:
One is mapStateToProps-prefer-hoisted
I have created a PR for that. This one flags all the "constant" objects created inside of mapStateToProps.

The other one is mapStateToProps-prefer-flat-props but this one depends on component implementation, so at most we could add a warning on mapStateToProps level.

[lencioni]

Ah interesting! I think the prefer-hoisted probably makes sense. I'm not entirely convinced about prefer-flat though, since that issue can often be solved via memoization, like with reselect. Same logic to detect, so probably just need to sort out the naming/documentation to make this clear. Not entirely sure what might be a good name though.

[DianaSuvorova]

Yeah, I am not sure about prefer-flat either. It is not always the best way to store props. Sometimes underlying structure makes a lot of sense. And yes, reselect seems like a better solution anyways.

let me see if @ljharb can take a look at my prefer-hoisted PR.

[ljharb]

Sorry for the delay :-) It's on my list to look at post-traveling. (altho it looks like you've already merged it, which is fine!)

[DianaSuvorova]

@ljharb , welcome back! Let me know if you have comments on the PR.