Web-stories-wp: ESLint: Prevent usage of Array index in keys

[miina]

Feature Description

We are currently using index as array keys in multiple places throughout the code, this, however, can potentially cause unexpected issues / unnecessary renders since an index as key behaves in a similar way as no key even though it doesn't display a warning.

See here in case you're interested in seeing a video comparing using no key vs index as key vs proper unique key.

The eslint rule: https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/no-array-index-key.md (Thanks @barklund for pointing out the rule exists)

Alternatives Considered

Additional Context

---

_Do not alter or remove anything below. The following sections will be managed by moderators only._

Acceptance Criteria

Implementation Brief

[swissspidy]

since an index as key behaves in a similar way as no key even though it doesn't display a warning.

I thought it's just that it causes issues when the array is sorted?

Either way, sounds like a no-brainer to add.

[miina]

I thought it's just that it causes issues when the array is sorted?

Can also cause issues when an element in the array is removed (unless it's the last element) or added (e.g. to the beginning).

[spacedmonkey]

I have done test to see how many error adding this lint. In short, there are lots of errors. 50+. See this output https://github.com/google/web-stories-wp/pull/4986/files

Fixing this in 50+ is no simple, as not all the arrays that are being mapped over, have a unique id to use as a key. @miina did you have any thoughts on this? Suppress these errors, remove key attribute or make a unique key.

[miina]

@spacedmonkey Looks like there are also some that are already unique but also include the index, so in some cases the index could be just removed.

For the other cases -- one option could be disabling these per line and creating 2 follow-up issues, one for the dashboard and one for the editor to fix the remaining ones. Thoughts? cc @barklund

[spacedmonkey]

For example this one just seem wrong.

The reason that the index was added as a key was to suppress another linting error, react/jsx-key. Here is an example.

 108:9  error  Missing "key" prop for element in iterator  react/jsx-key

I think in a way, it is either we go with react/no-array-index-key or react/jsx-key rule. There is going to be places where you are loop over data that doesn't not include a unique id. Or places, where the same unique id is reused, like the page id might be reused in a number of places.

This is an interesting read to explain a little more about this subject. Index as a key is an anti-pattern.

A possible workout would be use uuidv4 for keys. We already have uuid package installed and in use in other places. My only worry here is the performance overhead of generating these keys. This may be offset by some performance wins of using unique ids.

[swissspidy]

Array index as key is fine as long as the list stays the same. In those cases you can disable the rule for that code and add an explainer comment.

[spacedmonkey]

I am happy to merge the the lint change, but I think that dashboard and workspace teams should fix or comment out all the spaces in the code where the lint falls.

[swissspidy]

@spacedmonkey Makes sense to me. I fixed some of the low hanging fruits that I figured were safe to do so. Left a comment on the PR.

[spacedmonkey]

I have created follow up tickets https://github.com/google/web-stories-wp/issues/5017 https://github.com/google/web-stories-wp/issues/5018