Clarify note about stable keys

[yuyokk]

Hello,

I've been reading docs that say

If you don't provide stable keys (by using Math.random() for example), all the sub-trees are going to be re-rendered every single time. By giving the users the choice to choose the key, they have the ability to shoot themselves in the foot.

and then I see this comment on some issue that says

We made this mistake in one of our examples for a while and it's just really bad. By doing this, you'll actually end up recreating all of your nodes on every render because the key will be different.

Please let me know if it makes sense to remove this piece of doc from the site.

Thank you

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[jimfb]

I think the whole point of that sentence is to provide an example of unstable keys. It is explicitly calling out something that newbies sometimes try, and saying DON'T DO THIS. I don't think removing that parenthetical solves the problem.

If you really believe it should be more explicit, maybe we could say:

If you don't provide stable keys (by using Math.random() for example), all the sub-trees are going to be re-rendered every single time (which is bad - don't do this).

[yuyokk]

@jimfb oh, I see. Did not get it from the the first time. Thanks for clarification

[zpao]

I agree it doesn't read super well. Perhaps pulling the example out of the sentence.

If you don't provide stable keys, all the sub-trees are going to be re-rendered every single time. Math.random() is an example of an unstable key - it will be different on each render. By giving the users the choice to choose the key, they have the ability to shoot themselves in the foot.

[jimfb]

@zpao That still doesn't explicitly say it's bad to have unstable keys, so it's still subject to the miss-read that @yuyokk originally raised. It might not be obvious to a new user that re-rendering every time is a bad thing.

[quantizor]

Stable, predictable, unique keys for each rendered node optimize React's ability to do granular updates. Unstable keys (like those produced by Math.random()) will cause many nodes to be unnecessarily re-rendered per cycle and introduce a performance degradation.

How about that?

[jimfb]

Yeah, it's worse than just "bad performance" because you also loose internal state of all the decedents. Someone might be like "whatever, that's fine, performance is not an issue here" but then their code is actually wrong from a correctness perspective. Having said that, it's fine with me, subject to @zpao's go ahead.

Maybe:

Keys should be stable, predictable, and unique. Unstable keys (like those produced by Math.random()) will cause many nodes to be unnecessarily re-created, which can cause performance degradation and lost state in child components.

[quantizor]

I like that 👍 @jimfb

[zpao]

I like it, thanks @yaycmyk. Let's not use the word "node" though since it already has meaning in the DOM and React and neither of those is what you really mean here. All the terminology is confusing though… maybe just "component"? It's mostly correct since we use that to talk about instances. Or do whatever, this is definitely better regardless.

[quantizor]

Element? The key could be on a virtual <div> which isn't necessarily a component... at least not an intentional one.

[jimfb]

@yaycmyk Technically it is a component (just a trivially simple one). I'm fine with "component" or "node". Element probably isn't what we want (element refers to the JSX tag, which is always re-created regardless).

[quantizor]

Fair enough @jimfb. From a dev perspective I'd lean toward "nodes", as that reads more general and all-encompassing. When I hear the word "component", my first instinct is one that I've intentionally made via createClass or extending React.Component. Just my 0.02

[jimfb]

Yeah, that's why I'm fine with either one. Node is often used a little more loosely, but component is technically a little more accurate.

[TechyHouseWife557]

I do not remember signing on to any Contributor License Agreement In Facebook.  I would appreciate it very much if you can provide me with details as to when did  the signing up happen, IP address and what device was used to sign up. Please enlighten me about this activity because I seriously think someone else, without my permission is doing this. Thank you.Deo Larrijie O. GlovasaVirtual Administrative AssistantVirtual Dating Assistants (ViDA)+639281242567+1 (312) 789-4423---- On Thu, 05 Nov 2015 07:49:54 -0600 Facebook Community Botnotifications@github.com wrote ----Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!—Reply to this email directly or view it on GitHub.

[zpao]

@TechyHouseWife557 the post by that bot is directed at the author of the pull request, which is not you. I admit that isn't completely clear, perhaps we should include the author's GitHub username.

[jimfb]

Ping @yaycmyk: Do you want to update this PR so we can merge?

[quantizor]

@jimfb not my PR, but I'm happy to make a separate one with the revised text

[jimfb]

Oops, sorry @yaycmyk

Intended to ping @yuyokk

[yuyokk]

@jimfb sure. I will do it later today.

[facebook-github-bot]

@yuyokk updated the pull request.

[jimfb]

@yuyokk Looks good. Can you squash the two commits into a single commit (I use git rebase -i to squash and git push -f to push), and then I think we're ready to merge. Thanks!

[yuyokk]

@jimfb should be good now. Thanks!

[jimfb]

Excellent, thanks @yuyokk and @yaycmyk!