Add no-array-index-key rule #978
Conversation
lencioni
force-pushed
the
no-array-index-key
branch
from
1f65dd1 to
4a39e6c
Compare
| @@ -0,0 +1,23 @@ | |||
| # Prevent usage of Array index in keys | |||
|
|
|||
| Warn if an element uses an Array index in its `key`. | |||
There was a problem hiding this comment.
I think adding a short description of why you might want to enable this rule would be good. Something like:
It's a bad idea to use the array index since it doesn't uniquely identify your elements. In cases where the array is sorted, the index will be changed, even though the element representing that index may be the same -- resulting in unnecessary render cycles.
There was a problem hiding this comment.
Sure, I'll expand this document a little. I intended my next sentence to provide the reasoning, but perhaps it needs to be more explicit.
| if (callee.property.type !== 'Identifier') { | ||
| return null; | ||
| } | ||
| if (callee.property.name !== 'map') { |
There was a problem hiding this comment.
I would add forEach and filter in here as well. It's easy enough to support it for those rare cases where users would iterate over elements in different ways.
There was a problem hiding this comment.
I can see forEach for sure, but filter would be quite unusual. It should be easy enough to include though and I don't see any harm in it, so I'll add these.
|
Nice. Would be really great if this could also support |
|
All feedback has been addressed! PTAL |
| otherThings.push(<Hello key={index} />); | ||
| }); | ||
|
|
||
| things.some((thing, index) => { |
There was a problem hiding this comment.
what about reduce, reduceRight, find?
@ljharb What do you mean by this question? Are you referring to the name of the rule and documentation mentioning arrays? |
|
@lencioni i mean, |
|
@ljharb Yes, these will be caught as well for precisely the reason you mention. Likewise, so will any instance of any class that has one of these methods on it. This is what I was referring to about potential false positives in my commit message. |
This rule will warn when using the array index as the `key`. As a first pass, I decided to implement this for `Array.prototype.map`, since that is likely the most common case, but it theoretically could be expanded to find other cases so I kept the naming more generic. Like many rules, this one is imperfect and is prone to some false positives and negatives. For instance, if someone defines a `.map()` function on an object that doesn't have the same signature as `Array.prototype.map`, this will likely end up warning in those cases. However, I think the value of this rule outweighs its hypothetical drawbacks.
This makes the intention of this rule clearer.
Although some of these cover pretty unusual use-cases, they are easy enough to support and are unlikely to have false positives.
This helps to cover more possible use-cases that we want to prevent with this rule.
As @ljharb pointed out, this rule should spot array indexes when using reduce. Since the index is the third argument to the reduce callback, I needed to generalize some of my logic a bit to make this configurable for each type of function.
I missed examples of this in my first pass at this documentation.
lencioni
force-pushed
the
no-array-index-key
branch
from
de779d2 to
cff595e
Compare
|
@ljharb I believe I have addressed your feedback. |
This rule will warn when using the array index as the
key. As a firstpass, I decided to implement this for
Array.prototype.map, since thatis likely the most common case, but it theoretically could be expanded
to find other cases so I kept the naming more generic.
Like many rules, this one is imperfect and is prone to some false
positives and negatives. For instance, if someone defines a
.map()function on an object that doesn't have the same signature as
Array.prototype.map, this will likely end up warning in those cases.However, I think the value of this rule outweighs its hypothetical
drawbacks.