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

Fix Missing key Validation in React.Children #29675

Merged
merged 4 commits into from
May 31, 2024
Merged

Fix Missing key Validation in React.Children #29675

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1a6212d
Fix Missing `key` Validation in `React.Children`
yungsters May 30, 2024
2897d38
Fix Flow errors
yungsters May 30, 2024
63f6541
Prettier...
yungsters May 30, 2024
5d26f17
Restore `cloneAndReplaceKey` changes
yungsters May 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 15 additions & 5 deletions packages/react/src/ReactChildren.js
View file
Open in desktop
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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is weird that React.Children doesn't operate on portals. They don't get assigned keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I copied this from the $FlowFixMe comments in the lines above. 😅

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know but I found it interesting that 1) we didn't refine the type above 2) that we're checking isValidElement at all.

if (
nameSoFar !== '' &&
child != null &&
isValidElement(child) &&
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
View file
Open in desktop
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 />
Copy link
Contributor Author

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.

))}
</div>
);
}

const container = document.createElement('div');
const root = ReactDOMClient.createRoot(container);
await expect(async () => {
await act(() => {
root.render(
<ComponentRenderingMappedChildren>
{[<div />]}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

However, React does care if the supplied child is missing a key (and is in a list).

</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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

React is content with static child, even if mappedChild does not have a key.

</ComponentRenderingMappedChildren>,
);
});
}).toErrorDev([]);
});

it('warns for cloned list children without keys', async () => {
function ComponentRenderingClonedChildren({children}) {
return (
<div>
{React.Children.map(children, child => React.cloneElement(child))}
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 <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.

</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 () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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>;
}
Expand Down
7 changes: 1 addition & 6 deletions packages/react/src/jsx/ReactJSXElement.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -953,7 +953,7 @@ export function createElement(type, config, children) {
}

export function cloneAndReplaceKey(oldElement, newKey) {
const clonedElement = ReactElement(
return ReactElement(
Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand All @@ -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;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 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.

}
return clonedElement;
}

/**
Expand Down