Skip to content

Fixed bad key #1234

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

Fixed bad key #1234

wants to merge 1 commit into from

Conversation

ffxsam
Copy link

@ffxsam ffxsam commented Jan 14, 2016

Never use indices for keys in React. React depends on fully unique values for keys, so when items are added or removed, it can accurately shift them around in the DOM in the most efficient manner. The only instance where the user would see things go wrong is if you were using ReactCSSTransitionGroup to make items fade in and out as they appear/disappear. If you were to use the index as the key, you'd see some wonky things happen with the animation.

Never use indices for keys in React. React depends on fully unique values for keys, so when items are added or removed, it can accurately shift them around in the DOM in the most efficient manner. The only instance where the user would see things go wrong is if you were using `ReactCSSTransitionGroup` to make items fade in and out as they appear/disappear. If you were to use the index as the key, you'd see some wonky things happen with the animation.
@gaearon
Copy link
Contributor

gaearon commented Jan 15, 2016

Actually you can use indices as keys just fine if you don't have better IDs. Of course it breaks in some cases, but this is meant as a trivial example, not a real app. We don't expect people to take React components from our examples verbatim and add animation or other things to them.

That said the bigger issue is that example doesn't use proper IDs for todos. This causes other issues (#795, #1045). It will be fixed in #1186.

@gaearon gaearon closed this Jan 15, 2016
@rossipedia
Copy link

We don't expect people to take React components from our examples verbatim and add animation or other things to them.

Unfortunately, that's exactly what's going to happen in a lot of cases.

@ffxsam
Copy link
Author

ffxsam commented Jan 15, 2016

+1 to what @rossipedia said, and Dan, I think you totally missed that point. People new to React won't know any better, and think that's an appropriate way to key their lists. I actually thought it was acceptable too, and didn't realize it was a problem till my animated lists were behaving very oddly, then someone told me to not use indices as keys.

Tutorials are where beginners will either pick up good practices and habits, or develop bad ones.

@gaearon
Copy link
Contributor

gaearon commented Jan 15, 2016

I don't think I missed the point.

The approach suggested (id + text) defeats the whole purpose of React because it recreates DOM on every change. This is definitely not a good solution. In many cases it is a worse solution than just using indices, and we definitely won't recommend it in our guides because it completely destroys React performance.

As I said before the real problem is that todos don't have IDs. That is the correct fix. It is being fixed in #1186.

@ffxsam
Copy link
Author

ffxsam commented Jan 15, 2016

True, id + text is probably not great. However, just using text (temporarily, until you assign IDs) would even work fine - but just as a stopgap solution until an ID was put in place of course.

@gaearon
Copy link
Contributor

gaearon commented Jan 15, 2016

Using text will trip people up when they add editing. It's just a matter of breaking something else.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants