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

Add key validation to React.Children.toArray #11561

Closed
wants to merge 3 commits into from

Conversation

clemmy
Copy link
Contributor

@clemmy clemmy commented Nov 15, 2017

Addresses #11549

This warns only on React.Children.toArray. I don't think the other children helper methods (forEach, map, and count) are dangerous.

@gaearon
Copy link
Collaborator

gaearon commented Nov 15, 2017

What's the rollout plan? Just enabling this is going to be painful for everyone who's intentionally using to suppress the key warning.

@aweary
Copy link
Contributor

aweary commented Nov 15, 2017

I don't think the other children helper methods (forEach, map, and count) are dangerous.

It looks like map can also be used to skip key validation since it also uses mapIntoWithKeyPrefixInternal

@clemmy
Copy link
Contributor Author

clemmy commented Nov 15, 2017

@aweary Hmm, I imagined there would be a use case where .map is used to add keys to a child, but on second thought, that seems super awkward as you'd have to do something like:

React.Children.map(children, (child) => { <Child {...child.props} key={} /> })

@clemmy
Copy link
Contributor Author

clemmy commented Nov 15, 2017

@gaearon Add keys or use JSX fragments! Outside of that, they might just have to deal with the fact that since they're doing something weird, we're going to warn them.

@gaearon
Copy link
Collaborator

gaearon commented Nov 15, 2017

Have you tried this on www and assessed how many new warnings this will cause?

@facebook-github-bot
Copy link

Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired.

Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status.

@gaearon
Copy link
Collaborator

gaearon commented Jan 5, 2018

I'll close because it's a bit stale and also needs somebody to assess the number of new warnings at FB, but @clemmy's internship has ended. Maybe we'll revisit this some time in the future. I'll keep the issue open.

@gaearon gaearon closed this Jan 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants