Skip to content
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

Consider providing a default key for dynamic children #1342

Closed
frosas opened this issue Apr 1, 2014 · 6 comments
Closed

Consider providing a default key for dynamic children #1342

frosas opened this issue Apr 1, 2014 · 6 comments

Comments

@frosas
Copy link

frosas commented Apr 1, 2014

Because many times performance is not an issue, I find myself doing things like this:

React.DOM.ul(null, items.map(function(item) {
  return React.DOM.li({key: Math.random()}, item.property)
})

Would make sense to have something like this by default?

@zpao
Copy link
Member

zpao commented Apr 1, 2014

I understand where you're coming from, but please don't ever do what you have there. 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. While it might not hurt you today, it will one day.

A couple other points:

  • key is not really about performance, it's more about identity (which in turn leads to better performance). randomly assigned and changing values are not identity
  • We can't realistically provide keys without knowing how your data is modeled. I would suggest maybe using some sort of hashing function if you don't have ids
  • We already have internal keys when we use arrays, but they are the index in the array. When you insert a new element, those keys are wrong.

Now that said, they key warning can be pretty annoying. If it's driving people to end up with solutions like you have here, then we should probably spend some time thinking of a better way to do this.

tl;dr: "no"

@zpao zpao closed this as completed Apr 1, 2014
@frosas
Copy link
Author

frosas commented Apr 1, 2014

I'm not sure to understand what implications it has to not use a single key per children, apart from the degraded performance of course. Is it something related to future plans maybe?

I agree a random key is the worst key one can use, but it's still better than no key at all, and one can always spend time on a better solution when performance becomes an issue.

It's understandable you are trying to avoid this approach, I definitely do when working with entities but when the children components are based on value objects the alternatives doesn't seem much appealing (here I'm basically thinking about keeping a map of objects and their "ad hoc synthetic" keys).

@frosas
Copy link
Author

frosas commented Apr 1, 2014

Ok, I think I've got the identity point.

I could only think of the case of some state in a DOM element previously linked to another component being "inherited" when another similar component reuses the same element. My approach solved it well enough (IMHO).

But now I see that, if the key is not only unique but persistent over time, the opposite case still applies: every time the component is rendered all the state not being handled by React simply disappears!

Hopefully in the future it will be easier to handle these cases with ES6 WeakMaps.

@sophiebits
Copy link
Collaborator

I agree a random key is the worst key one can use, but it's still better than no key at all.

If no keys are provided, React uses the index in the array which is almost always better than a random key.

@suhaotian
Copy link

@spicyj Yeah,I agree

@noscripter
Copy link

#1342 (comment)

We already have internal keys when we use arrays, but they are the index in the array. When you insert a new element, those keys are wrong.

This point is important to note, because it's a common problem to be encountered when writing list components.

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

No branches or pull requests

5 participants