Perform duplicate key validation in a separate pass

gaearon · gaearon · commit 572465774d2c · 2016-12-14T13:53:34.000Z
This avoids complicating the main loops and will be easier to port if that code gets forked into more special cases.
I also added tests specifically for iterables since they weren't being tested.

The tree hook test that used to fail now passes because we create a Set lazily and so it doesn't trigger that code path. We'll revisit this when we decide how to polyfill.
diff --git a/scripts/fiber/tests-failing.txt b/scripts/fiber/tests-failing.txt
@@ -62,7 +62,6 @@ src/renderers/shared/__tests__/ReactPerf-test.js
 
 src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js
 * can be retrieved by ID
-* works
 
 src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js
 * gets recorded during an update
diff --git a/scripts/fiber/tests-passing-except-dev.txt b/scripts/fiber/tests-passing-except-dev.txt
@@ -111,6 +111,7 @@ src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js
 * reports update counts
 * does not report top-level wrapper as a root
 * registers inlined text nodes
+* works
 
 src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js
 * gets recorded for host roots
diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt
@@ -1280,8 +1280,10 @@ src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js
 * gets ignored if new html is equal
 
 src/renderers/shared/shared/__tests__/ReactChildReconciler-test.js
-* warns for duplicated keys
-* warns for duplicated keys with component stack info
+* warns for duplicated array keys
+* warns for duplicated array keys with component stack info
+* warns for duplicated iterable keys
+* warns for duplicated iterable keys with component stack info
 
 src/renderers/shared/shared/__tests__/ReactComponent-test.js
 * should throw when supplying a ref outside of render method
@@ -1425,7 +1427,8 @@ src/renderers/shared/shared/__tests__/ReactMultiChild-test.js
 * should replace children with different constructors
 * should NOT replace children with different owners
 * should replace children with different keys
-* should warn for duplicated keys with component stack info
+* should warn for duplicated array keys with component stack info
+* should warn for duplicated iterable keys with component stack info
 * should reorder bailed-out children
 
 src/renderers/shared/shared/__tests__/ReactMultiChildReconcile-test.js
diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js
@@ -550,39 +550,44 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
   function warnOnDuplicateKey(
     child : mixed,
     returnFiber : Fiber,
-    knownKeys : Set<string>
-  ) {
+    knownKeys : Set<string> | null
+  ) : Set<string> | null {
     if (__DEV__) {
-      if (child == null || typeof child !== 'object') {
-        return;
+      if (typeof child !== 'object' || child == null) {
+        return knownKeys;
       }
       switch (child.$$typeof) {
-        // They have keys.
         case REACT_ELEMENT_TYPE:
         case REACT_COROUTINE_TYPE:
         case REACT_YIELD_TYPE:
         case REACT_PORTAL_TYPE:
           const key = child.key;
           if (typeof key !== 'string') {
-            return;
+            break;
           }
-          if (knownKeys.has(key)) {
-            warning(
-              false,
-              'Encountered two children with the same key, ' +
-              '`%s`. Child keys must be unique; when two children share a key, ' +
-              'only the first child will be used.%s',
-              key,
-              getStackAddendumByFiber(returnFiber)
-            );
-          } else {
+          if (knownKeys == null) {
+            knownKeys = new Set();
+            knownKeys.add(key);
+            break;
+          }
+          if (!knownKeys.has(key)) {
             knownKeys.add(key);
+            break;
           }
-          return;
+          warning(
+            false,
+            'Encountered two children with the same key, ' +
+            '`%s`. Child keys must be unique; when two children share a key, ' +
+            'only the first child will be used.%s',
+            key,
+            getStackAddendumByFiber(returnFiber)
+          );
+          break;
         default:
-          return;
+          break;
       }
     }
+    return knownKeys;
   }
 
   function reconcileChildrenArray(
@@ -610,11 +615,18 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
     // If you change this code, also update reconcileChildrenIterator() which
     // uses the same algorithm.
 
+    if (__DEV__) {
+      // First, validate keys.
+      let knownKeys = null;
+      for (let i = 0; i < newChildren.length; i++) {
+        const child = newChildren[i];
+        knownKeys = warnOnDuplicateKey(child, returnFiber, knownKeys);
+      }
+    }
+
     let resultingFirstChild : ?Fiber = null;
     let previousNewFiber : ?Fiber = null;
 
-    let knownKeysInDev = null;
-
     let oldFiber = currentFirstChild;
     let lastPlacedIndex = 0;
     let newIdx = 0;
@@ -628,15 +640,10 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
           nextOldFiber = oldFiber.sibling;
         }
       }
-      const newChild = newChildren[newIdx];
-      if (__DEV__) {
-        knownKeysInDev = knownKeysInDev || new Set();
-        warnOnDuplicateKey(newChild, returnFiber, knownKeysInDev);
-      }
       const newFiber = updateSlot(
         returnFiber,
         oldFiber,
-        newChild,
+        newChildren[newIdx],
         priority
       );
       if (!newFiber) {
@@ -647,18 +654,6 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
         if (!oldFiber) {
           oldFiber = nextOldFiber;
         }
-        if (__DEV__) {
-          if (
-            knownKeysInDev != null &&
-            newChild != null &&
-            typeof newChild.key === 'string'
-          ) {
-            // Since we break out of this loop without incrementing newIdx,
-            // we will look at this child again in the next loop.
-            // Forget we checked it to avoid a false positive for duplicate key.
-            knownKeysInDev.delete(newChild.key);
-          }
-        }
         break;
       }
       if (shouldTrackSideEffects) {
@@ -693,14 +688,9 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
       // If we don't have any more existing children we can choose a fast path
       // since the rest will all be insertions.
       for (; newIdx < newChildren.length; newIdx++) {
-        const newChild = newChildren[newIdx];
-        if (__DEV__) {
-          knownKeysInDev = knownKeysInDev || new Set();
-          warnOnDuplicateKey(newChild, returnFiber, knownKeysInDev);
-        }
         const newFiber = createChild(
           returnFiber,
-          newChild,
+          newChildren[newIdx],
           priority
         );
         if (!newFiber) {
@@ -723,16 +713,11 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
 
     // Keep scanning and use the map to restore deleted items as moves.
     for (; newIdx < newChildren.length; newIdx++) {
-      const newChild = newChildren[newIdx];
-      if (__DEV__) {
-        knownKeysInDev = knownKeysInDev || new Set();
-        warnOnDuplicateKey(newChild, returnFiber, knownKeysInDev);
-      }
       const newFiber = updateFromMap(
         existingChildren,
         returnFiber,
         newIdx,
-        newChild,
+        newChildren[newIdx],
         priority
       );
       if (newFiber) {
@@ -769,17 +754,40 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
   function reconcileChildrenIterator(
     returnFiber : Fiber,
     currentFirstChild : ?Fiber,
-    newChildren : Iterator<*>,
+    newChildrenIterable : Iterable<*>,
     priority : PriorityLevel) : ?Fiber {
 
     // This is the same implementation as reconcileChildrenArray(),
     // but using the iterator instead.
 
+    const iteratorFn = getIteratorFn(newChildrenIterable);
+    if (typeof iteratorFn !== 'function') {
+      throw new Error('An object is not an iterable.');
+    }
+
+    if (__DEV__) {
+      // First, validate keys.
+      // We'll get a different iterator later for the main pass.
+      const newChildren = iteratorFn.call(newChildrenIterable);
+      if (newChildren == null) {
+        throw new Error('An iterable object provided no iterator.');
+      }
+      let knownKeys = null;
+      let step = newChildren.next();
+      for (; !step.done; step = newChildren.next()) {
+        const child = step.value;
+        knownKeys = warnOnDuplicateKey(child, returnFiber, knownKeys);
+      }
+    }
+
+    const newChildren = iteratorFn.call(newChildrenIterable);
+    if (newChildren == null) {
+      throw new Error('An iterable object provided no iterator.');
+    }
+
     let resultingFirstChild : ?Fiber = null;
     let previousNewFiber : ?Fiber = null;
 
-    let knownKeysInDev = null;
-
     let oldFiber = currentFirstChild;
     let lastPlacedIndex = 0;
     let newIdx = 0;
@@ -795,15 +803,10 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
           nextOldFiber = oldFiber.sibling;
         }
       }
-      const newChild = step.value;
-      if (__DEV__) {
-        knownKeysInDev = knownKeysInDev || new Set();
-        warnOnDuplicateKey(newChild, returnFiber, knownKeysInDev);
-      }
       const newFiber = updateSlot(
         returnFiber,
         oldFiber,
-        newChild,
+        step.value,
         priority
       );
       if (!newFiber) {
@@ -814,18 +817,6 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
         if (!oldFiber) {
           oldFiber = nextOldFiber;
         }
-        if (__DEV__) {
-          if (
-            knownKeysInDev != null &&
-            newChild != null &&
-            typeof newChild.key === 'string'
-          ) {
-            // Since we break out of this loop without incrementing newIdx,
-            // we will look at this child again in the next loop.
-            // Forget we checked it to avoid a false positive for duplicate key.
-            knownKeysInDev.delete(newChild.key);
-          }
-        }
         break;
       }
       if (shouldTrackSideEffects) {
@@ -860,14 +851,9 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
       // If we don't have any more existing children we can choose a fast path
       // since the rest will all be insertions.
       for (; !step.done; newIdx++, step = newChildren.next()) {
-        const newChild = step.value;
-        if (__DEV__) {
-          knownKeysInDev = knownKeysInDev || new Set();
-          warnOnDuplicateKey(newChild, returnFiber, knownKeysInDev);
-        }
         const newFiber = createChild(
           returnFiber,
-          newChild,
+          step.value,
           priority
         );
         if (!newFiber) {
@@ -890,16 +876,11 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
 
     // Keep scanning and use the map to restore deleted items as moves.
     for (; !step.done; newIdx++, step = newChildren.next()) {
-      const newChild = step.value;
-      if (__DEV__) {
-        knownKeysInDev = knownKeysInDev || new Set();
-        warnOnDuplicateKey(newChild, returnFiber, knownKeysInDev);
-      }
       const newFiber = updateFromMap(
         existingChildren,
         returnFiber,
         newIdx,
-        newChild,
+        step.value,
         priority
       );
       if (newFiber) {
@@ -1172,16 +1153,11 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
         );
       }
 
-      const iteratorFn = getIteratorFn(newChild);
-      if (iteratorFn) {
-        const iterator = iteratorFn.call(newChild);
-        if (iterator == null) {
-          throw new Error('An iterable object provided no iterator.');
-        }
+      if (getIteratorFn(newChild)) {
         return reconcileChildrenIterator(
           returnFiber,
           currentFirstChild,
-          iterator,
+          newChild,
           priority
         );
       }
diff --git a/src/renderers/shared/shared/__tests__/ReactChildReconciler-test.js b/src/renderers/shared/shared/__tests__/ReactChildReconciler-test.js
diff --git a/src/renderers/shared/shared/__tests__/ReactMultiChild-test.js b/src/renderers/shared/shared/__tests__/ReactMultiChild-test.js
