-
Notifications
You must be signed in to change notification settings - Fork 47.6k
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
Fix Missing key
Validation in React.Children
#29675
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -868,7 +868,105 @@ describe('ReactChildren', () => { | |
]); | ||
}); | ||
|
||
it('should warn for flattened children lists', async () => { | ||
it('warns for mapped list children without keys', async () => { | ||
function ComponentRenderingMappedChildren({children}) { | ||
return ( | ||
<div> | ||
{React.Children.map(children, child => ( | ||
<div /> | ||
))} | ||
</div> | ||
); | ||
} | ||
|
||
const container = document.createElement('div'); | ||
const root = ReactDOMClient.createRoot(container); | ||
await expect(async () => { | ||
await act(() => { | ||
root.render( | ||
<ComponentRenderingMappedChildren> | ||
{[<div />]} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. However, React does care if the supplied |
||
</ComponentRenderingMappedChildren>, | ||
); | ||
}); | ||
}).toErrorDev([ | ||
'Warning: Each child in a list should have a unique "key" prop.', | ||
]); | ||
}); | ||
|
||
it('does not warn for mapped static children without keys', async () => { | ||
function ComponentRenderingMappedChildren({children}) { | ||
return ( | ||
<div> | ||
{React.Children.map(children, child => ( | ||
<div /> | ||
))} | ||
</div> | ||
); | ||
} | ||
|
||
const container = document.createElement('div'); | ||
const root = ReactDOMClient.createRoot(container); | ||
await expect(async () => { | ||
await act(() => { | ||
root.render( | ||
<ComponentRenderingMappedChildren> | ||
<div /> | ||
<div /> | ||
Comment on lines
+914
to
+915
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. React is content with static |
||
</ComponentRenderingMappedChildren>, | ||
); | ||
}); | ||
}).toErrorDev([]); | ||
}); | ||
|
||
it('warns for cloned list children without keys', async () => { | ||
function ComponentRenderingClonedChildren({children}) { | ||
return ( | ||
<div> | ||
{React.Children.map(children, child => React.cloneElement(child))} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Seeking input on whether to keep this and the test case below, or to delete them. |
||
</div> | ||
); | ||
} | ||
|
||
const container = document.createElement('div'); | ||
const root = ReactDOMClient.createRoot(container); | ||
await expect(async () => { | ||
await act(() => { | ||
root.render( | ||
<ComponentRenderingClonedChildren> | ||
{[<div />]} | ||
</ComponentRenderingClonedChildren>, | ||
); | ||
}); | ||
}).toErrorDev([ | ||
'Warning: Each child in a list should have a unique "key" prop.', | ||
]); | ||
}); | ||
|
||
it('does not warn for cloned static children without keys', async () => { | ||
function ComponentRenderingClonedChildren({children}) { | ||
return ( | ||
<div> | ||
{React.Children.map(children, child => React.cloneElement(child))} | ||
</div> | ||
); | ||
} | ||
|
||
const container = document.createElement('div'); | ||
const root = ReactDOMClient.createRoot(container); | ||
await expect(async () => { | ||
await act(() => { | ||
root.render( | ||
<ComponentRenderingClonedChildren> | ||
<div /> | ||
<div /> | ||
</ComponentRenderingClonedChildren>, | ||
); | ||
}); | ||
}).toErrorDev([]); | ||
}); | ||
|
||
it('warns for flattened list children without keys', async () => { | ||
function ComponentRenderingFlattenedChildren({children}) { | ||
return <div>{React.Children.toArray(children)}</div>; | ||
} | ||
|
@@ -888,7 +986,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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I realized while reading more code that we refer to these as "static" children, so this is just a rename. |
||
function ComponentRenderingFlattenedChildren({children}) { | ||
return <div>{React.Children.toArray(children)}</div>; | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a revert of #29662, which is no longer necessary. |
||
oldElement.type, | ||
newKey, | ||
// When enableRefAsProp is on, this argument is ignored. This check only | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If the original element was There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When I started tackling this, I was also considering copying this logic to I'll revert this change for now, but I mention it to ask whether you think If you think there's something to do there, I'd be happy to follow up. |
||
} | ||
return clonedElement; | ||
} | ||
|
||
/** | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React actually does not care if
mappedChild
is missing a key.