Merge pull request facebook#75 from gaearon/fix-double-add

Fix double-adding fibers when traversing
bvaughn · Apr 5, 2019 · 387ad54 · 387ad54
2 parents 0c8129c + 2a5b8a9
commit 387ad54
Show file tree

Hide file tree

Showing 4 changed files with 90 additions and 60 deletions.
diff --git a/shells/dev/app/Toggle/index.js b/shells/dev/app/Toggle/index.js
@@ -0,0 +1,21 @@
+import React, { useState } from 'react';
+
+export default function Toggle() {
+  const [show, setShow] = useState(false);
+  return (
+    <>
+      <h2>Toggle</h2>
+      <div>
+        <>
+          <button onClick={() => setShow(s => !s)}>Show child</button>
+          {show && ' '}
+          {show && <Greeting>Hello</Greeting>}
+        </>
+      </div>
+    </>
+  );
+}
+
+function Greeting({ children }) {
+  return <p>{children}</p>;
+}
diff --git a/shells/dev/app/index.js b/shells/dev/app/index.js
@@ -10,6 +10,7 @@ import ElementTypes from './ElementTypes';
 import InspectableElements from './InspectableElements';
 import InteractionTracing from './InteractionTracing';
 import ToDoList from './ToDoList';
+import Toggle from './Toggle';
 
 import './styles.css';
 
@@ -31,6 +32,7 @@ function mountTestApp() {
   mountHelper(InspectableElements);
   mountHelper(ElementTypes);
   mountHelper(EditableProps);
+  mountHelper(Toggle);
   mountHelper(DeeplyNestedComponents);
 }
 

diff --git a/src/backend/renderer.js b/src/backend/renderer.js
@@ -743,23 +743,26 @@ export function attach(
     }
   }
 
-  function mountFiber(fiber: Fiber, parentFiber: Fiber | null) {
+  function mountFiber(
+    fiber: Fiber,
+    parentFiber: Fiber | null,
+    traverseSiblings = false
+  ) {
     if (__DEBUG__) {
       debug('mountFiber()', fiber, parentFiber);
     }
 
     const shouldEnqueueMount = !shouldFilterFiber(fiber);
-
     if (shouldEnqueueMount) {
       enqueueMount(fiber, parentFiber);
     }
 
     if (fiber.child !== null) {
-      mountFiber(fiber.child, shouldEnqueueMount ? fiber : parentFiber);
+      mountFiber(fiber.child, shouldEnqueueMount ? fiber : parentFiber, true);
     }
 
-    if (fiber.sibling) {
-      mountFiber(fiber.sibling, parentFiber);
+    if (traverseSiblings && fiber.sibling !== null) {
+      mountFiber(fiber.sibling, parentFiber, true);
     }
   }
 

diff --git a/src/devtools/store.js b/src/devtools/store.js
@@ -482,37 +482,39 @@ export default class Store extends EventEmitter {
 
           i = i + 3;
 
+          if (this._idToElement.has(id)) {
+            throw new Error(
+              'Store already contains fiber ' +
+                id +
+                '. This is a bug in React DevTools.'
+            );
+          }
+
           if (type === ElementTypeRoot) {
             if (__DEBUG__) {
               debug('Add', `new root fiber ${id}`);
             }
 
-            if (this._idToElement.has(id)) {
-              // The renderer's tree walking approach sometimes mounts the same Fiber twice with Suspense and Lazy.
-              // For now, we avoid adding it to the tree twice by checking if it's already been mounted.
-              // Maybe in the future we'll revisit this.
-            } else {
-              const supportsProfiling = operations[i] > 0;
-              i++;
-
-              this._roots = this._roots.concat(id);
-              this._rootIDToRendererID.set(id, rendererID);
-              this._rootIDToCapabilities.set(id, { supportsProfiling });
-
-              this._idToElement.set(id, {
-                children: [],
-                depth: -1,
-                displayName: null,
-                id,
-                key: null,
-                ownerID: 0,
-                parentID: 0,
-                type,
-                weight: 0,
-              });
-
-              haveRootsChanged = true;
-            }
+            const supportsProfiling = operations[i] > 0;
+            i++;
+
+            this._roots = this._roots.concat(id);
+            this._rootIDToRendererID.set(id, rendererID);
+            this._rootIDToCapabilities.set(id, { supportsProfiling });
+
+            this._idToElement.set(id, {
+              children: [],
+              depth: -1,
+              displayName: null,
+              id,
+              key: null,
+              ownerID: 0,
+              parentID: 0,
+              type,
+              weight: 0,
+            });
+
+            haveRootsChanged = true;
           } else {
             parentID = ((operations[i]: any): number);
             i++;
@@ -545,40 +547,42 @@ export default class Store extends EventEmitter {
               );
             }
 
-            if (this._idToElement.has(id)) {
-              // The renderer's tree walking approach sometimes mounts the same Fiber twice with Suspense and Lazy.
-              // For now, we avoid adding it to the tree twice by checking if it's already been mounted.
-              // Maybe in the future we'll revisit this.
-            } else {
-              parentElement = ((this._idToElement.get(parentID): any): Element);
-              parentElement.children = parentElement.children.concat(id);
-
-              const element: Element = {
-                children: [],
-                depth: parentElement.depth + 1,
-                displayName,
-                id,
-                key,
-                ownerID,
-                parentID: parentElement.id,
-                type,
-                weight: 1,
-              };
-
-              this._idToElement.set(id, element);
-
-              const oldAddedElementIDs = addedElementIDs;
-              addedElementIDs = new Uint32Array(addedElementIDs.length + 1);
-              addedElementIDs.set(oldAddedElementIDs);
-              addedElementIDs[oldAddedElementIDs.length] = id;
-
-              weightDelta = 1;
-            }
+            parentElement = ((this._idToElement.get(parentID): any): Element);
+            parentElement.children = parentElement.children.concat(id);
+
+            const element: Element = {
+              children: [],
+              depth: parentElement.depth + 1,
+              displayName,
+              id,
+              key,
+              ownerID,
+              parentID: parentElement.id,
+              type,
+              weight: 1,
+            };
+
+            this._idToElement.set(id, element);
+
+            const oldAddedElementIDs = addedElementIDs;
+            addedElementIDs = new Uint32Array(addedElementIDs.length + 1);
+            addedElementIDs.set(oldAddedElementIDs);
+            addedElementIDs[oldAddedElementIDs.length] = id;
+
+            weightDelta = 1;
           }
           break;
         case TREE_OPERATION_REMOVE:
           id = ((operations[i + 1]: any): number);
 
+          if (!this._idToElement.has(id)) {
+            throw new Error(
+              'Store does not contain fiber ' +
+                id +
+                '. This is a bug in React DevTools.'
+            );
+          }
+
           i = i + 2;
 
           element = ((this._idToElement.get(id): any): Element);