Fix Missing key Validation in React.Children

facebook · May 30, 2024 · 1a6212d · 1a6212d
1 parent 9710853
commit 1a6212d
Show file tree

Hide file tree

Showing 3 changed files with 110 additions and 13 deletions.
diff --git a/packages/react/src/ReactChildren.js b/packages/react/src/ReactChildren.js
@@ -229,11 +229,15 @@ 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`.
+          if (nameSoFar !== '' && 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;
             }
           }

diff --git a/packages/react/src/__tests__/ReactChildren-test.js b/packages/react/src/__tests__/ReactChildren-test.js
@@ -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>;
     }
@@ -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>;
     }

diff --git a/packages/react/src/jsx/ReactJSXElement.js b/packages/react/src/jsx/ReactJSXElement.js
@@ -953,7 +953,7 @@ export function createElement(type, config, children) {
 }
 
 export function cloneAndReplaceKey(oldElement, newKey) {
-  const clonedElement = ReactElement(
+  return ReactElement(
     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;
-  }
-  return clonedElement;
 }
 
 /**