Fix Missing key Validation in React.Children
#29675
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| return ( | ||
| <div> | ||
| {React.Children.map(children, child => ( | ||
| <div /> |
There was a problem hiding this comment.
React actually does not care if mappedChild is missing a key.
| await act(() => { | ||
| root.render( | ||
| <ComponentRenderingMappedChildren> | ||
| {[<div />]} |
There was a problem hiding this comment.
However, React does care if the supplied child is missing a key (and is in a list).
| <div /> | ||
| <div /> |
There was a problem hiding this comment.
React is content with static child, even if mappedChild does not have a key.
| function ComponentRenderingClonedChildren({children}) { | ||
| return ( | ||
| <div> | ||
| {React.Children.map(children, child => React.cloneElement(child))} |
There was a problem hiding this comment.
This is the use case that I originally sought to fix.
I suppose this is now more or less the same as the test case above (that simply returns <div />), but maybe still worth keeping as a test case?
Seeking input on whether to keep this and the test case below, or to delete them.
| @@ -888,7 +988,7 @@ describe('ReactChildren', () => { | |||
| ]); | |||
| }); | |||
|
|
|||
| it('does not warn for flattened positional children', async () => { | |||
| it('does not warn for flattened static children without keys', async () => { | |||
There was a problem hiding this comment.
I realized while reading more code that we refer to these as "static" children, so this is just a rename.
| @@ -953,7 +953,7 @@ export function createElement(type, config, children) { | |||
| } | |||
|
|
|||
| export function cloneAndReplaceKey(oldElement, newKey) { | |||
| const clonedElement = ReactElement( | |||
| return ReactElement( | |||
There was a problem hiding this comment.
This is a revert of #29662, which is no longer necessary.
|
Comparing: 9710853...5d26f17 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
| @@ -966,11 +966,6 @@ export function cloneAndReplaceKey(oldElement, newKey) { | |||
| __DEV__ && enableOwnerStacks ? oldElement._debugStack : undefined, | |||
| __DEV__ && enableOwnerStacks ? oldElement._debugTask : undefined, | |||
| ); | |||
| if (__DEV__) { | |||
| // The cloned element should inherit the original element's key validation. | |||
| clonedElement._store.validated = oldElement._store.validated; | |||
There was a problem hiding this comment.
If the original element was validated === 2 because it came from Flight or another Children helper, then was passed through map again, then it turns into validated === 0 but since it does have a key now it wouldn't issue an error even though it should've.
There was a problem hiding this comment.
When I started tackling this, I was also considering copying this logic to React.cloneElement.
I'll revert this change for now, but I mention it to ask whether you think React.cloneElement should also persist the original element's _store.validated (and whether that is affected by a new key being supplied to React.cloneElement).
If you think there's something to do there, I'd be happy to follow up.
| // print the warning later. | ||
| // If `child` was an element without a `key`, we need to validate if | ||
| // it should have had a `key`, before assigning one to `mappedChild`. | ||
| // $FlowFixMe[incompatible-type] Flow incorrectly thinks React.Portal doesn't have a key |
There was a problem hiding this comment.
It is weird that React.Children doesn't operate on portals. They don't get assigned keys.
There was a problem hiding this comment.
To be honest, I copied this from the $FlowFixMe comments in the lines above. 😅
There was a problem hiding this comment.
I know but I found it interesting that 1) we didn't refine the type above 2) that we're checking isValidElement at all.
|
Thanks for the quick review! |
| if ( | ||
| nameSoFar !== '' && | ||
| child != null && | ||
| isValidElement(child) && |
There was a problem hiding this comment.
Now that I'm looking closer, it seems like we're extracting a key from Portals with getElementKey.
I believe that this check should really check if is element or is portal.
There was a problem hiding this comment.
I guess Portals don't have stores and they don't run the validation in ChildFiber so this is not necessary right now, and it's probably not worth adding key validation for portals after the fact.
## Summary In #29088, the validation logic for `React.Children` inspected whether `mappedChild` — the return value of the map callback — has a valid `key`. However, this deviates from existing behavior which only warns if the original `child` is missing a required `key`. This fixes false positive `key` validation warnings when using `React.Children`, by validating the original `child` instead of `mappedChild`. This is a more general fix that expands upon my previous fix in #29662. ## How did you test this change? ``` $ yarn test ReactChildren-test.js ``` DiffTrain build for commit 8fd963a.
## Summary In #29088, the validation logic for `React.Children` inspected whether `mappedChild` — the return value of the map callback — has a valid `key`. However, this deviates from existing behavior which only warns if the original `child` is missing a required `key`. This fixes false positive `key` validation warnings when using `React.Children`, by validating the original `child` instead of `mappedChild`. This is a more general fix that expands upon my previous fix in #29662. ## How did you test this change? ``` $ yarn test ReactChildren-test.js ``` DiffTrain build for [8fd963a](8fd963a)
Summary
In #29088, the validation logic for
React.Childreninspected whethermappedChild— the return value of the map callback — has a validkey. However, this deviates from existing behavior which only warns if the originalchildis missing a requiredkey.This fixes false positive
keyvalidation warnings when usingReact.Children, by validating the originalchildinstead ofmappedChild.This is a more general fix that expands upon my previous fix in #29662.
How did you test this change?