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.

Sign up for GitHub

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

Jump to bottom

Regression in null key handling in 0.12 (possibly intentional) #2386

Closed
sophiebits opened this issue Oct 20, 2014 · 6 comments · Fixed by #2388
Closed

Regression in null key handling in 0.12 (possibly intentional) #2386

sophiebits opened this issue Oct 20, 2014 · 6 comments · Fixed by #2388
Milestone
0.12

Comments

@sophiebits
Copy link
Collaborator

In 0.11, the code

<div>
  <div key={null}>foo</div>
  <div key={null}>bar</div>
</div>

assigned implicit keys to both children. In 0.12, it gives the error

Warning: flattenChildren(...): Encountered two children with the same key, .$null. Child keys must be unique; when two children share a key, only the first child will be used.

and renders only "foo", because the key is cast to a string in ReactElement

react/src/core/ReactElement.js

Line 140 in be468c2

key = config.key === undefined ? null : '' + config.key;

before it gets checked against null in traverseAllChildren:

react/src/utils/traverseAllChildren.js

Line 50 in be468c2

if (component && component.key != null) {

I guess it's okay if this is intentional but it broke a part of KA in the upgrade.

@sebmarkbage? (cc @jdan)
@sophiebits sophiebits added this to the 0.12 milestone Oct 20, 2014
@sebmarkbage
Copy link
Collaborator

hm. Yea, this was intentional to ensure more stable types through out the system.

I don't think null should be treated as a valid "non-value" to align it with defaultProps, default arguments and default values in destructuring. The intention of "null" is to symbolize a missing object. However, this can be of any type.

It's weird that undefined is coerced into null. It should be left as undefined.

For refs, we expect them to become objects so it is valid to have null there to represent a missing value.

We have a two options:

  1. Allow any value to be coerced to strings.
  2. Throw on non-string values.
  3. Don't coerce to string and start using maps as keys. E.g. Dates would become by reference instead of by value. null would be treated as a key and would still be duplicate.

@sophiebits
Copy link
Collaborator Author

Maybe we can support null meaning no key for a 0.12 with a warning, then drop support in 0.13? Just trying to avoid breaking things (that are impossible to grep for) without a warning. (I have no idea if we have other places in our code that do this by accident…)

@sebmarkbage
Copy link
Collaborator

They don't in prod though, right? I thought we had a fallback for duplicate values that would be essentially the same?

@sophiebits
Copy link
Collaborator Author

No, that code previously outputted foo bar and outputs just foo with 0.12. We ignore all children after the first with a given key.

@sebmarkbage
Copy link
Collaborator

oh. fine, warning for one version and then swap behavior. Want to send a PR?

@sophiebits
Copy link
Collaborator Author

Will do.

@sophiebits sophiebits mentioned this issue Oct 21, 2014
Merged
sophiebits added a commit to sophiebits/react that referenced this issue Oct 21, 2014
@sophiebits
Support but warn on key={null}
dd92786 
Fixes facebook#2386.

Test Plan: jest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.12
Development

Successfully merging a pull request may close this issue.

Support but warn on key={null} sophiebits/react
2 participants
@sophiebits @sebmarkbage