New rule to avoid prop value with array or object creation in render

[jer-sen]

There is a rule for prop value with function creation (bind, function(...), or arrow function): react/jsx-no-bind But I can't find a rule for array ([]) or object ({}) creation.

It would be useful since it would help to avoid useless renders of pure components.

[ljharb]

React (and generically, anything in JS) already knows how to optimize comparison of arrays and objects; the hazard doesn't exist.

In other words, passing a new [] every time shouldn't trigger a render in a pure component.

[jer-sen]

React only does a shallow comparison of this.props so a pure component will be rerendered.

Just have a look to this example if you don't believe me: https://jsfiddle.net/69z2wepo/97573/

So what about this new rule ?

[ljharb]

Interesting, I thought PureComponent was smarter than that.

@gaearon is there any chance PureComponent could/should be made a bit smarter, to check one level deeper for arrays and objects?

[gaearon]

I don’t think it’s very likely we’ll change it to do deeper checks. That’s both hard to explain/debug and can be inefficient.

[ljharb]

Thanks, in that case, a rule for this is a great idea.

[alexzherdev]

I'd be happy to take a shot at a new rule for this. It occurred to me that this could also take an ignoreDOMComponents option à la jsx-no-bind?

[alexzherdev]

While I'm preparing the PR, I could use some suggestions on the name. jsx-no-allocation-in-props? jsx-no-objects-arrays-in-props? @ljharb

[ljharb]

It wouldn't be confined to jsx, and it's specifically dealing with avoiding triggering unnecessary rerenders.

This means that there's no point in ever checking DOM components, since they can never be pure (altho it should still have an ignore option for custom components).

So, for a name: something about avoiding new object (including arrays and functions) creation in props (which includes children) passed to custom components in the render path. I'm not sure what would be a succinct name, but I'll think about it.

[alexzherdev]

Hmm I was thinking of it as an analog to jsx-no-bind which already handles function creation. (I ended up using a lot of code from there) Are you picturing something different? Also, even with DOM components there's still an issue with excessive GC, right?

[ljharb]

I'm picturing it as a superset of no-bind, that could eventually replace it, that more instructively and correctly deals with the true hazard: excessive rerenders.

GC isn't really the issue in practice.

[alexzherdev]

Oh, I see. That might be more ambitious than what I got from the issue description (array/object literals). Would it be reasonable for this rule to at least start with that? Also, to avoid spamming this issue, would you be open to reviewing a PR with tests first to settle on the scope?

[ljharb]

Definitely reasonable to start with that :-) and yes, a PR starting with tests is great.

[alexzherdev]

@ljharb sorry for the delay on this. Had too much fun writing some lint rules for a hackathon at work 😊 I will push what I have shortly and put my thoughts in the PR.

[sbaechler]

Please add the option to only report an error if it actually triggered a re-render. E.g. as a prop to a PureComponent or a lambda function as a prop to a class component. But a lambda function as prop to a function component or a HTML element would not cause an error.

I think a rule like this would be more useful than jsx-no-bind. This rule should also catch Array.map or Array.filter in mapStateToProps(), which I see quite often in code reviews.

[ljharb]

@sbaechler there’s no way to statically know that; it’d have to error on every non-html element.

There’s also no way for any eslint rule to reliably catch any instance methods of anything, including map/filter.

[sbaechler]

@ljharb Would it be possible to enforce in the Type Script plugin? Otherwise I am afraid that this rule overshoots its goal since it really only matters for PureComponent. It could even lead to developers starting to mutate objects just to comply with it.

If eslint cannot detect map/filter, then the naming proposed in #2024 (no-update-forcing-props) is misleading. It might leave developers in false security since it only catches the most obvious of cases.

[ljharb]

@sbaechler it only matters for pure components or any component that implements sCU or any component that might do those things at any time in the future. I’m not sure how you’d be able to enforce it at the consumer site with or without types; the component itself would have to declare that it’s pure, and the linter plugin would have to reach across files to determine that.