Skip to content

Commit

Permalink
Fix Missing key Validation in React.Children (#29675)
Browse files Browse the repository at this point in the history
## 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
```
  • Loading branch information
yungsters authored May 31, 2024
1 parent aa3d6c0 commit 8fd963a
Show file tree
Hide file tree
Showing 2 changed files with 115 additions and 7 deletions.
20 changes: 15 additions & 5 deletions packages/react/src/ReactChildren.js
Original file line number Diff line number Diff line change
Expand Up @@ -229,11 +229,21 @@ function mapIntoArray(
childKey,
);
if (__DEV__) {
if (nameSoFar !== '' && mappedChild.key == null) {
// We need to validate that this child should have had a key before assigning it one.
if (!newChild._store.validated) {
// We mark this child as having failed validation but we let the actual renderer
// 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
if (
nameSoFar !== '' &&
child != null &&
isValidElement(child) &&
child.key == null
) {
// We check truthiness of `child._store.validated` instead of being
// inequal to `1` to provide a bit of backward compatibility for any
// libraries (like `fbt`) which may be hacking this property.
if (child._store && !child._store.validated) {
// Mark this child as having failed validation, but let the actual
// renderer print the warning later.
newChild._store.validated = 2;
}
}
Expand Down
102 changes: 100 additions & 2 deletions packages/react/src/__tests__/ReactChildren-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 />]}
</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 />
</ComponentRenderingMappedChildren>,
);
});
}).toErrorDev([]);
});

it('warns for cloned list 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 />]}
</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>;
}
Expand All @@ -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 () => {
function ComponentRenderingFlattenedChildren({children}) {
return <div>{React.Children.toArray(children)}</div>;
}
Expand Down

0 comments on commit 8fd963a

Please sign in to comment.