Eradicate the 'unique "key" prop' warnings

[JoelMarcey]

For the guides and Components/APIs, we would continuously get:

Warning: Each child in an array or iterator should have a unique "key" prop.

when rendering them.

This add key attributes to our DOM elements that did not have them in order
to remove this warning. It uses a random string generator to create unique keys.

Should we establish the key a different or better way?

I went full hammer on this and put key attributes on everything. May have
been overkill, but the warnings are gone.

Should I be more selective on what we put the key attribute on?

Also fixed a:

Warning: Unknown DOM property frameborder. Did you mean frameBorder?

issue as well.

Test Plan:

• No more warnings

- Guides still render the same - APIs still render the same (when applied with fix from https://github.com//pull/8746)

[ghost]

By analyzing the blame information on this pull request, we identified @caabernathy and @janicduplessis to be potential reviewers.

[lacker]

I think adding a random key is not a good idea since no-key behaves the same as random-key. If you add random keys everywhere you will make rerendering slower because now it can't reuse things with the existing heuristics. The only place we should need a key is when we are putting stuff in an array - if you're just returning some jsx then you shouldn't need a key. See https://facebook.github.io/react/docs/reconciliation.html for more on this. I think it would be ok to either 1/ abandon this and live with the warning or 2/ figure out some sane key to put on the places where we do have a number of siblings.

[lacker]

more discussion of keys here: facebook/react#1342

[JoelMarcey]

I hate the warning. It looks a bit clowny to have that on our own server :)

So, instead of the general hammer, we can try to be selective, like you suggest.

[JoelMarcey]

So random keys are always frowned upon it seems like. That may be an issue with the way we currently have the GettingStarted page toggler and stuff because even if we use static keys, we get duplicate key warnings as well.

Random keys solve that problem, but if they are bad, then they are bad. So gotta think about a better way.

[ide]

Could you use a counter to generate the keys deterministically? Without looking closely, I'd expect these components not to render stateful DOM elements (they're not <input> components for example) so the key prop isn't actually important.

The idea is that the counter is just a variable defined at the top of render() and keys within the render method are just counter++. This way, two render() passes without any state changes will deterministically produce the same output, keeping render() idempotent.

[ide]

Also most of those places with randomKey() in the diff don't need keys at all. We should define keys just where they're needed so they don't mislead people reading the code later.

[JoelMarcey]

Also most of those places with randomKey() in the diff don't need keys at all. We should define keys just where they're needed so they don't mislead people reading the code later.

Yeah, I have a local commit that reduces the number of keys only to those that don't produce the warnings.

[JoelMarcey]

@ide Could consider a counter of some sort, yes. Thanks for the idea. I still fear that the toggler we have for Getting Started might cause problems with a constant key. I will have to see.

[JoelMarcey]

I think we are on to something with the sequential key counter, @ide! Thanks! :D

[JoelMarcey]

@ide I am noticing that as I test every doc in our docs, when I think I have gotten rid of the warning, I click on another one that gives the warning because it has an element not found in another document (say blockquote or something).

Is there a reason NOT to just put a key on all DOM elements to future proof this problem? Instead of selectively putting a key on all the elements that we use now that are causing the warning?

[ide]

Is there a reason NOT to just put a key on all DOM elements to future proof this problem? Instead of selectively putting a key on all the elements that we use now that are causing the warning?

That would probably work though I think it would be better to have spurious warnings than spurious keys.

[lacker]

I agree it would be better to have spurious warnings than spurious keys. The "right way" to handle this is to assign keys only when appropriate, and to never use random keys. So if someone someday goes and does this the right way, if we add random keys everywhere they'll just have to figure out which ones to undo. If we leave the warnings, then the warnings will tell this someone where keys are needed. The warnings only are relevant if you look at the console, right?

[JoelMarcey]

Ok. Well this can serve as a model to get started with in the future. I think you should have the sequentialKey module where you fix one or two places where warnings arise. Then you can have small subsequent pull requests to fix others that come up after that.