Fix Missing `key` Validation in `React.Children` (#29675)
## Summary In https://github.com/facebook/react/pull/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 https://github.com/facebook/react/pull/29662. ## How did you test this change? ``` $ yarn test ReactChildren-test.js ```
Timothy Yung committed
May 30, 2024 at 18:02 UTC
8fd963a1e5ec89459cac27fb1d9ad193a0604110
2 files changed
+115
-7
packages/react/src/ReactChildren.js
+15
-5
@@ -229,11 +229,21 @@ function mapIntoArray(
229
childKey,
230
);
231
if (__DEV__) {
232
- if (nameSoFar !== '' && mappedChild.key == null) {
233
- // We need to validate that this child should have had a key before assigning it one.
234
- if (!newChild._store.validated) {
235
- // We mark this child as having failed validation but we let the actual renderer
236
- // print the warning later.
232
+ // If `child` was an element without a `key`, we need to validate if
233
+ // it should have had a `key`, before assigning one to `mappedChild`.
234
+ // $FlowFixMe[incompatible-type] Flow incorrectly thinks React.Portal doesn't have a key
235
+ if (
236
+ nameSoFar !== '' &&
237
+ child != null &&
238
+ isValidElement(child) &&
239
+ child.key == null
240
+ ) {
241
+ // We check truthiness of `child._store.validated` instead of being
242
+ // inequal to `1` to provide a bit of backward compatibility for any
243
+ // libraries (like `fbt`) which may be hacking this property.
244
+ if (child._store && !child._store.validated) {
245
+ // Mark this child as having failed validation, but let the actual
246
+ // renderer print the warning later.
247
newChild._store.validated = 2;
248
}
249
}
packages/react/src/__tests__/ReactChildren-test.js
+100
-2
@@ -868,7 +868,105 @@ describe('ReactChildren', () => {
868
]);
869
});
870
871
- it('should warn for flattened children lists', async () => {
871
+ it('warns for mapped list children without keys', async () => {
872
+ function ComponentRenderingMappedChildren({children}) {
873
+ return (
874
+ <div>
875
+ {React.Children.map(children, child => (
876
+ <div />
877
+ ))}
878
+ </div>
879
+ );
880
+ }
881
+
882
+ const container = document.createElement('div');
883
+ const root = ReactDOMClient.createRoot(container);
884
+ await expect(async () => {
885
+ await act(() => {
886
+ root.render(
887
+ <ComponentRenderingMappedChildren>
888
+ {[<div />]}
889
+ </ComponentRenderingMappedChildren>,
890
+ );
891
+ });
892
+ }).toErrorDev([
893
+ 'Warning: Each child in a list should have a unique "key" prop.',
894
+ ]);
895
+ });
896
+
897
+ it('does not warn for mapped static children without keys', async () => {
898
+ function ComponentRenderingMappedChildren({children}) {
899
+ return (
900
+ <div>
901
+ {React.Children.map(children, child => (
902
+ <div />
903
+ ))}
904
+ </div>
905
+ );
906
+ }
907
+
908
+ const container = document.createElement('div');
909
+ const root = ReactDOMClient.createRoot(container);
910
+ await expect(async () => {
911
+ await act(() => {
912
+ root.render(
913
+ <ComponentRenderingMappedChildren>
914
+ <div />
915
+ <div />
916
+ </ComponentRenderingMappedChildren>,
917
+ );
918
+ });
919
+ }).toErrorDev([]);
920
+ });
921
+
922
+ it('warns for cloned list children without keys', async () => {
923
+ function ComponentRenderingClonedChildren({children}) {
924
+ return (
925
+ <div>
926
+ {React.Children.map(children, child => React.cloneElement(child))}
927
+ </div>
928
+ );
929
+ }
930
+
931
+ const container = document.createElement('div');
932
+ const root = ReactDOMClient.createRoot(container);
933
+ await expect(async () => {
934
+ await act(() => {
935
+ root.render(
936
+ <ComponentRenderingClonedChildren>
937
+ {[<div />]}
938
+ </ComponentRenderingClonedChildren>,
939
+ );
940
+ });
941
+ }).toErrorDev([
942
+ 'Warning: Each child in a list should have a unique "key" prop.',
943
+ ]);
944
+ });
945
+
946
+ it('does not warn for cloned static children without keys', async () => {
947
+ function ComponentRenderingClonedChildren({children}) {
948
+ return (
949
+ <div>
950
+ {React.Children.map(children, child => React.cloneElement(child))}
951
+ </div>
952
+ );
953
+ }
954
+
955
+ const container = document.createElement('div');
956
+ const root = ReactDOMClient.createRoot(container);
957
+ await expect(async () => {
958
+ await act(() => {
959
+ root.render(
960
+ <ComponentRenderingClonedChildren>
961
+ <div />
962
+ <div />
963
+ </ComponentRenderingClonedChildren>,
964
+ );
965
+ });
966
+ }).toErrorDev([]);
967
+ });
968
+
969
+ it('warns for flattened list children without keys', async () => {
970
function ComponentRenderingFlattenedChildren({children}) {
971
return <div>{React.Children.toArray(children)}</div>;
972
}
@@ -888,7 +986,7 @@ describe('ReactChildren', () => {
986
]);
987
});
988
891
- it('does not warn for flattened positional children', async () => {
989
+ it('does not warn for flattened static children without keys', async () => {
990
function ComponentRenderingFlattenedChildren({children}) {
991
return <div>{React.Children.toArray(children)}</div>;
992
}