Cleaned up renderer code and added additional unit test

Author: Brian Vaughn

packages/react-devtools-shared/src/__tests__/store-test.js

@@ -1075,43 +1075,110 @@ describe('Store', () => {
       `);
     });
 
-    it('during passive get counted (after a delay)', () => {
-      function Example() {
-        React.useEffect(() => {
-          console.error('test-only: passive error');
-          console.warn('test-only: passive warning');
-        });
-        return null;
+    describe('during passive effects', () => {
+      function flushPendingBridgeOperations() {
+        jest.runOnlyPendingTimers();
       }
-      const container = document.createElement('div');
-
-      withErrorsOrWarningsIgnored(['test-only:'], () => {
-        act(() => {
-          ReactDOM.render(<Example />, container);
-          // flush bridge operations
-          jest.runOnlyPendingTimers();
-        }, false);
-      });
 
-      expect(store).toMatchInlineSnapshot(`
-        [root]
-            <Example>
-      `);
-
-      // flush count after delay
-      act(() => {
+      // Gross abstraction around pending passive warning/error delay.
+      function flushPendingPassiveErrorAndWarningCounts() {
         jest.advanceTimersByTime(1000);
-      }, false);
+      }
 
-      // After a delay, passive effects should be committed as well
-      expect(store).toMatchInlineSnapshot(`
-        ✕ 1, ⚠ 1
-        [root]
-            <Example> ✕⚠
-      `);
+      it('are counted (after a delay)', () => {
+        function Example() {
+          React.useEffect(() => {
+            console.error('test-only: passive error');
+            console.warn('test-only: passive warning');
+          });
+          return null;
+        }
+        const container = document.createElement('div');
+
+        withErrorsOrWarningsIgnored(['test-only:'], () => {
+          act(() => {
+            ReactDOM.render(<Example />, container);
+          }, false);
+        });
+        flushPendingBridgeOperations();
+        expect(store).toMatchInlineSnapshot(`
+          [root]
+              <Example>
+        `);
+
+        // After a delay, passive effects should be committed as well
+        act(flushPendingPassiveErrorAndWarningCounts, false);
+        expect(store).toMatchInlineSnapshot(`
+          ✕ 1, ⚠ 1
+          [root]
+              <Example> ✕⚠
+        `);
+
+        act(() => ReactDOM.unmountComponentAtNode(container));
+        expect(store).toMatchInlineSnapshot(``);
+      });
 
-      act(() => ReactDOM.unmountComponentAtNode(container));
-      expect(store).toMatchInlineSnapshot(``);
+      it('are flushed early when there is a new commit', () => {
+        function Example() {
+          React.useEffect(() => {
+            console.error('test-only: passive error');
+            console.warn('test-only: passive warning');
+          });
+          return null;
+        }
+
+        function Noop() {
+          return null;
+        }
+
+        const container = document.createElement('div');
+
+        withErrorsOrWarningsIgnored(['test-only:'], () => {
+          act(() => {
+            ReactDOM.render(
+              <>
+                <Example />
+              </>,
+              container,
+            );
+          }, false);
+          flushPendingBridgeOperations();
+          expect(store).toMatchInlineSnapshot(`
+            [root]
+                <Example>
+          `);
+
+          // Before warnings and errors have flushed, flush another commit.
+          act(() => {
+            ReactDOM.render(
+              <>
+                <Example />
+                <Noop />
+              </>,
+              container,
+            );
+          }, false);
+          flushPendingBridgeOperations();
+          expect(store).toMatchInlineSnapshot(`
+            ✕ 1, ⚠ 1
+            [root]
+                <Example> ✕⚠
+                <Noop>
+          `);
+        });
+
+        // After a delay, passive effects should be committed as well
+        act(flushPendingPassiveErrorAndWarningCounts, false);
+        expect(store).toMatchInlineSnapshot(`
+          ✕ 2, ⚠ 2
+          [root]
+              <Example> ✕⚠
+              <Noop>
+        `);
+
+        act(() => ReactDOM.unmountComponentAtNode(container));
+        expect(store).toMatchInlineSnapshot(``);
+      });
     });
 
     it('from react get counted', () => {

packages/react-devtools-shared/src/backend/renderer.js

@@ -631,7 +631,8 @@ export function attach(
     // In this case, we should wait until the rest of the passive effects have run,
     // but we shouldn't wait until the next commit because that might be a long time.
     // This would also cause "tearing" between an inspected Component and the tree view.
-    // Then again we don't want to flush too soon because rendering might stil be going on.
+    // Then again we don't want to flush too soon because this could be an error during async rendering.
+    // Use a debounce technique to ensure that we'll eventually flush.
     flushPendingErrorsAndWarningsAfterDelay();
   }
 
@@ -1205,10 +1206,12 @@ export function attach(
     }
   }
 
-  const pendingOperations: Array<number> = [];
+  type OperationsArray = Array<number>;
+
+  const pendingOperations: OperationsArray = [];
   const pendingRealUnmountedIDs: Array<number> = [];
   const pendingSimulatedUnmountedIDs: Array<number> = [];
-  let pendingOperationsQueue: Array<Array<number>> | null = [];
+  let pendingOperationsQueue: Array<OperationsArray> | null = [];
   const pendingStringTable: Map<string, number> = new Map();
   let pendingStringTableLength: number = 0;
   let pendingUnmountedRootID: number | null = null;
@@ -1225,55 +1228,56 @@ export function attach(
     pendingOperations.push(op);
   }
 
+  function flushOrQueueOperations(operations: OperationsArray): void {
+    if (pendingOperationsQueue !== null) {
+      pendingOperationsQueue.push(operations);
+    } else {
+      hook.emit('operations', operations);
+    }
+  }
+
   let flushPendingErrorsAndWarningsAfterDelayTimeoutID = null;
-  function flushPendingErrorsAndWarningsAfterDelay() {
+
+  function clearPendingErrorsAndWarningsAfterDelay() {
     if (flushPendingErrorsAndWarningsAfterDelayTimeoutID !== null) {
       clearTimeout(flushPendingErrorsAndWarningsAfterDelayTimeoutID);
+      flushPendingErrorsAndWarningsAfterDelayTimeoutID = null;
     }
+  }
+
+  function flushPendingErrorsAndWarningsAfterDelay() {
+    clearPendingErrorsAndWarningsAfterDelay();
 
     flushPendingErrorsAndWarningsAfterDelayTimeoutID = setTimeout(() => {
       flushPendingErrorsAndWarningsAfterDelayTimeoutID = null;
 
       if (pendingOperations.length > 0) {
-        // On the off chance that somethign else has pushed pending operations,
+        // On the off chance that something else has pushed pending operations,
         // we should bail on warnings; it's probably not safe to push midway.
         return;
       }
 
       recordPendingErrorsAndWarnings();
 
       if (pendingOperations.length === 0) {
-        // No warnings or errors to flush.
+        // No warnings or errors to flush; we can bail out early here too.
         return;
       }
 
-      const operations = new Array(2 + 1 + pendingOperations.length);
-
-      // Identify which renderer this update is coming from.
-      // This enables roots to be mapped to renderers,
-      // Which in turn enables fiber props, states, and hooks to be inspected.
-      let i = 0;
-      operations[i++] = rendererID;
-      operations[i++] = currentRootID; // Use this ID in case the root was unmounted!
-      operations[i++] = 0; // String table size
-
-      // Fill in the rest of the operations.
+      // We can create a smaller operations array than flushPendingEvents()
+      // because we only need to flush warning and error counts.
+      // Only a few pieces of fixed information are required up front.
+      const operations: OperationsArray = new Array(
+        3 + pendingOperations.length,
+      );
+      operations[0] = rendererID;
+      operations[1] = currentRootID;
+      operations[2] = 0; // String table size
       for (let j = 0; j < pendingOperations.length; j++) {
-        operations[i + j] = pendingOperations[j];
+        operations[3 + j] = pendingOperations[j];
       }
 
-      // Let the frontend know about tree operations.
-      // The first value in this array will identify which root it corresponds to,
-      // so we do no longer need to dispatch a separate root-committed event.
-      if (pendingOperationsQueue !== null) {
-        // Until the frontend has been connected, store the tree operations.
-        // This will let us avoid walking the tree later when the frontend connects,
-        // and it enables the Profiler's reload-and-profile functionality to work as well.
-        pendingOperationsQueue.push(operations);
-      } else {
-        // If we've already connected to the frontend, just pass the operations through.
-        hook.emit('operations', operations);
-      }
+      flushOrQueueOperations(operations);
 
       pendingOperations.length = 0;
     }, 1000);
@@ -1291,10 +1295,7 @@ export function attach(
   }
 
   function recordPendingErrorsAndWarnings() {
-    if (flushPendingErrorsAndWarningsAfterDelayTimeoutID !== null) {
-      clearTimeout(flushPendingErrorsAndWarningsAfterDelayTimeoutID);
-      flushPendingErrorsAndWarningsAfterDelayTimeoutID = null;
-    }
+    clearPendingErrorsAndWarningsAfterDelay();
 
     fibersWithChangedErrorOrWarningCounts.forEach(fiberID => {
       const fiber = idToFiberMap.get(fiberID);
@@ -1385,7 +1386,7 @@ export function attach(
     // Which in turn enables fiber props, states, and hooks to be inspected.
     let i = 0;
     operations[i++] = rendererID;
-    operations[i++] = currentRootID; // Use this ID in case the root was unmounted!
+    operations[i++] = currentRootID;
 
     // Now fill in the string table.
     // [stringTableLength, str1Length, ...str1, str2Length, ...str2, ...]
@@ -1432,18 +1433,9 @@ export function attach(
     i += pendingOperations.length;
 
     // Let the frontend know about tree operations.
-    // The first value in this array will identify which root it corresponds to,
-    // so we do no longer need to dispatch a separate root-committed event.
-    if (pendingOperationsQueue !== null) {
-      // Until the frontend has been connected, store the tree operations.
-      // This will let us avoid walking the tree later when the frontend connects,
-      // and it enables the Profiler's reload-and-profile functionality to work as well.
-      pendingOperationsQueue.push(operations);
-    } else {
-      // If we've already connected to the frontend, just pass the operations through.
-      hook.emit('operations', operations);
-    }
+    flushOrQueueOperations(operations);
 
+    // Reset all of the pending state now that we've told the frontend about it.
     pendingOperations.length = 0;
     pendingRealUnmountedIDs.length = 0;
     pendingSimulatedUnmountedIDs.length = 0;