Remove react/no-array-index-key comments

[marcalexiei]

Follow to discussion in #3270

For every eslint-disable-next-line react/no-array-index-key we should find a valid string or number instead of using an index

[ckifer]

After changing some of these lint warnings there are quite a few instances of this throwing non-unique key warnings as shown in #4004

pie sectors, legend items, tooltip items (anything that has been changed recently).

It seems storybook swallows these errors so its easier to identify them in the storybook. CC @imagineLife

[BuddhikaGeasman]

@ckifer If this is not resolved yet. Any chance I could take it?

[ckifer]

@BuddhikaGeasman there was a recent report of an issue in the latest comments here #4004

If you can find the error mentioned there that would be more important than removing these lint ignores. I actually think removing these ignores is impossible without re-thinking some things. Any time we use data to try and address these there are duplicate key errors

(But yes, any contributions are welcome of course!)

[DrJShoff]

This error also appears in scatter chart when dots are drawn on top of each other. This causes duplications of dots if the chart is resized or the data updated as the chart can't keep track of all dots as they have the same key.

[imagineLife]

Perhaps components that render chartable items should have (more) required unique props.

Uniquely Identify Items

One recurring error in duplicate keys is revealed when 2 chartable items (pie slices, dots on a scatter, bars, could be more) have the same cartable "data": x values, y values, etc.

One idea to tackle this seems to be set more required props for renderable elements: something like a "name", or "id". There does not seem to be enough required props that an be leveraged for a unique key when iterating over data to render.

Another idea could be to add a required prop called uniqueId (or key or something) where developers who use recharges must bring-their-own key to recharts. This approach would perhaps be a bit more “breaking”. Recharts, as the library, would “pass along” the key provided by the developer to the charting rendering. The benefit here would be that recharts stops “caring about" the key the way it is currently trying to.

[ckifer]

Imo we should only put this burden on the consumer if they are iterating through things themselves. If we iterate, we need to be the ones who ensure keys are unique.

set more required props for renderable elements

The consumer shouldn't have to have more data in their data in order to render a chart without errors

developers who use recharts must bring-their-own key to recharts

This doesn't work because that key will be the same for every element of every component giving us the same problem we currently have

Not really sure of a great way - we can hash the whole object but that has the same chances of collision. We can generate a UUID based of the object but same thing, we can use a UUID but that negates the purpose...

[imagineLife]

hey @ckifer thanks for the detailed reply! 😄

I do think iterating over items is such a normal practice for react developers that "passing along" the unique-key react "problem" along to the developer could work out one way or another. I do also get that recharts is doing the mapping, not consumers explicitly - it would be nice (&/or expected?!) if recharts could handle the mapping without the index - based on the data &/or some other uniquely-identifying prop(s) per element.
(I also happen to know that other react-charting libs use the index too, so at least recharts is not alone here 😵)

[ckifer]

I agree and users can do mapping themselves in some components using Cell.

I guess what I want to drive home is that we need tests that protect against these errors before we can make changes like this. Not that I don't want to or we shouldn't, but we need a guarantee for all valid cases that the key we create is actually unique