Skip to content

Respect 'key' prop for object identity#591

Merged
petehunt merged 4 commits into
facebook:masterfrom
sophiebits:mount-key
Dec 26, 2013
Merged

Respect 'key' prop for object identity#591
petehunt merged 4 commits into
facebook:masterfrom
sophiebits:mount-key

Conversation

@sophiebits
Copy link
Copy Markdown
Collaborator

Now when a key prop appears, its value is always honored. This means that in the root component or as an only child, changing key will cause remounting; in a children object, the key prop will be joined with the object key to form a two-part key.

Fixes #590.

cc @ide

@sebmarkbage
Copy link
Copy Markdown
Contributor

The current semantics of keys is that they're only used to identify components within an array, as a convenience to avoid creating a keyed object fragment. They're not general purpose state breakers.

It might make sense to change the semantics to general purpose state breakers since it's kind of confusing that this honors the key:

var child = <StatefulComponent key="foo" />;
return <div>{child}</div>

But this doesn't:

return {child}

Which makes it a refactoring hazard. In that case it should probably use the key of the component in the position of an object fragment. E.g. this should also respect the key of the object position AND the child's key (the effective resulting key would be {bar}{foo}).

return { bar: child };

@jordwalke This type of keying convention, of putting values on the props, can completely avoid the deoptimization problem since there is no need for these keys to ever go on a flattened children object, as your linked-list implementation shows.

@jordwalke
Copy link
Copy Markdown
Contributor

@sebmarkbage : But they go on an object, which appears to be enough to cause the optimizer to go into a tailspin.

@sophiebits
Copy link
Copy Markdown
Collaborator Author

@sebmarkbage Seems reasonable, done.

@zpao
Copy link
Copy Markdown
Member

zpao commented Dec 6, 2013

@sebmarkbage Can you handle reviewing and syncing this one?

Comment thread src/core/shouldUpdateReactComponent.js Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way you treat index here rubs me the wrong way... if you do it this way I believe you're baking additional assumptions about the caller of this function (that it's called on two components in the same position). While that was true before it seems like a bit of a weird assumption to bake in, no?.

Can you change getComponentKey() to look at _mountIndex or does that break stuff?

Now when a `key` prop appears, its value is always honored. This means that in the root component or as an only child, changing key will cause remounting; in a `children` object, the `key` prop will be joined with the object key to form a two-part key.

Fixes facebook#590.
...and inline getComponentKey into traverseAllChildren.js.
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.

Honor instance identity (the "key" prop) for the root component

5 participants