Require key prop in array comprehensions

[philip-peterson]

Problem
Per #1345, a key prop should be provided whenever there is an array of children elements. We do not currently see an error or warning when one is not provided.

Questionnaire

• I'm interested in fixing this myself but don't know where to start
• I would like to fix and I have a solution
• I don't have time to fix this right now, but maybe later

[philip-peterson]

Slight tangent, but React insists on throwing a warning whenever you use a key that is simply the index of the object being displayed. I would suggest we don't do that in Yew, since sometimes (even in React) the developer has no choice as the index is the only thing that would indicate continuity of the element being rendered.

[siku2]

Slight tangent, but React insists on throwing a warning whenever you use a key that is simply the index of the object being displayed. I would suggest we don't do that in Yew, since sometimes (even in React) the developer has no choice as the index is the only thing that would indicate continuity of the element being rendered.

Isn't this contradicting the point of requiring the key prop in the first place?
In React, using the index as a key is the same as not using a key at all because the default key is the index.
If we don't emit a warning for that then I don't think we should emit a warning for omitting the key in the first place.
What do you think?

Perhaps there's a way for Yew to identify situations where a key makes sense and only warn the user about those situations?

[philip-peterson]

It may perhaps be functionally equivalent behavior, but explicitly requiring the developer to set a key prop forces them to think about whether the index is really the best key for the job. In many cases, there is an id readily available, so that is the natural choice.

[siku2]

Having the user explicitly set the key prop to remind them of its importance is a good point.
I think we should discuss how we want to go about doing that though.
Panicking seems like a bad choice to me (for debug builds it might be okay but definitely not for release builds).
Another option would be to make it a compile-time check. Personally, I believe this to be the best choice but it might be slightly less user-friendly (particularly for "quick hack" projects).
Lastly, we could emit a warning like React does (At least I think that's what React does). The problem there is that it's easy to miss if you don't look at the console.
Do you have a preference?

[philip-peterson]

Could we enforce it with a compile time error, plus a "quick hack" escape hatch similar to lint ignores? Then we could disallow those escape hatches for production builds but allow it + emit a warning for dev builds.

[siku2]

This should be possible using proc_macro_diagnostics which are still unstable. We could abort with an error for now and switch to a suppressible lint error in the future

[WorldSEnder]

I think not all lists immediately deserve a warning for not using keys, or at least such a lint should be in the pedantic category. Two situations come to mind that are almost never false positives:

• Some of the list items use keys, some don't. This is almost surely not intended, since it's just ignoring all the keys, anyway.
• Using a map(..).collect() style concatenation probably intends to use keys if the thing mapped over is state and not const.

Requiring keys everywhere is, as said before, overkill and will lead to a lot of false positives and I'd only include it if there's a pedantic error category and at least part of the view function generates "dynamic" layout instead of always the same order of child components/elements.