refactor[devtools/extension]: more stable element updates polling to avoid timed out errors

Author: hoxyq

packages/react-devtools-extensions/src/background/index.js

@@ -83,7 +83,7 @@ chrome.runtime.onConnect.addListener(port => {
   }
 
   if (isNumeric(port.name)) {
-    // Extension port doesn't have tab id specified, because its sender is the extension.
+    // DevTools page port doesn't have tab id specified, because its sender is the extension.
     const tabId = +port.name;
 
     registerTab(tabId);
@@ -231,3 +231,20 @@ chrome.runtime.onMessage.addListener((message, sender) => {
     }
   }
 });
+
+chrome.tabs.onActivated.addListener(({tabId: activeTabId}) => {
+  for (const registeredTabId in ports) {
+    if (
+      ports[registeredTabId].proxy != null &&
+      ports[registeredTabId].extension != null
+    ) {
+      const numericRegisteredTabId = +registeredTabId;
+      const event =
+        activeTabId === numericRegisteredTabId
+          ? 'resumeElementPolling'
+          : 'pauseElementPolling';
+
+      ports[registeredTabId].extension.postMessage({event});
+    }
+  }
+});

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

@@ -461,10 +461,6 @@ describe('InspectedElement', () => {
     // This test causes an intermediate error to be logged but we can ignore it.
     jest.spyOn(console, 'error').mockImplementation(() => {});
 
-    // Wait for our check-for-updates poll to get the new data.
-    jest.runOnlyPendingTimers();
-    await Promise.resolve();
-
     // Clear the frontend cache to simulate DevTools being closed and re-opened.
     // The backend still thinks the most recently-inspected element is still cached,
     // so the frontend needs to tell it to resend a full value.
@@ -1072,7 +1068,6 @@ describe('InspectedElement', () => {
       await TestUtilsAct(async () => {
         await TestRendererAct(async () => {
           inspectElementPath(path);
-          jest.runOnlyPendingTimers();
         });
       });
 
@@ -1227,7 +1222,6 @@ describe('InspectedElement', () => {
       await TestUtilsAct(async () => {
         await TestRendererAct(async () => {
           inspectElementPath(path);
-          jest.runOnlyPendingTimers();
         });
       });
 
@@ -1309,7 +1303,6 @@ describe('InspectedElement', () => {
       await TestUtilsAct(async () => {
         await TestRendererAct(async () => {
           inspectElementPath(path);
-          jest.runOnlyPendingTimers();
         });
       });
 
@@ -1470,9 +1463,8 @@ describe('InspectedElement', () => {
 
     async function loadPath(path) {
       await TestUtilsAct(async () => {
-        await TestRendererAct(async () => {
+        await TestRendererAct(() => {
           inspectElementPath(path);
-          jest.runOnlyPendingTimers();
         });
       });
 
@@ -1597,9 +1589,8 @@ describe('InspectedElement', () => {
 
     async function loadPath(path) {
       await TestUtilsAct(async () => {
-        await TestRendererAct(async () => {
+        await TestRendererAct(() => {
           inspectElementPath(path);
-          jest.runOnlyPendingTimers();
         });
       });
 
@@ -1640,9 +1631,11 @@ describe('InspectedElement', () => {
     expect(inspectedElement.props).toMatchInlineSnapshot(`
       {
         "nestedObject": {
-          "a": Dehydrated {
-            "preview_short": {…},
-            "preview_long": {b: {…}, value: 2},
+          "a": {
+            "b": {
+              "value": 2,
+            },
+            "value": 2,
           },
           "value": 2,
         },

packages/react-devtools-shared/src/backendAPI.js

@@ -11,6 +11,7 @@ import {hydrate, fillInPath} from 'react-devtools-shared/src/hydration';
 import {separateDisplayNameAndHOCs} from 'react-devtools-shared/src/utils';
 import Store from 'react-devtools-shared/src/devtools/store';
 import TimeoutError from 'react-devtools-shared/src/errors/TimeoutError';
+import ElementPollingCancellationError from 'react-devtools-shared/src/errors/ElementPollingCancellationError';
 
 import type {
   InspectedElement as InspectedElementBackend,
@@ -138,7 +139,7 @@ export function storeAsGlobal({
   });
 }
 
-const TIMEOUT_DELAY = 5000;
+const TIMEOUT_DELAY = 10_000;
 
 let requestCounter = 0;
 
@@ -151,10 +152,17 @@ function getPromiseForRequestID<T>(
   return new Promise((resolve, reject) => {
     const cleanup = () => {
       bridge.removeListener(eventType, onInspectedElement);
+      bridge.removeListener('shutdown', onDisconnect);
+      bridge.removeListener('pauseElementPolling', onDisconnect);
 
       clearTimeout(timeoutID);
     };
 
+    const onDisconnect = () => {
+      cleanup();
+      reject(new ElementPollingCancellationError());
+    };
+
     const onInspectedElement = (data: any) => {
       if (data.responseID === requestID) {
         cleanup();
@@ -168,6 +176,8 @@ function getPromiseForRequestID<T>(
     };
 
     bridge.addListener(eventType, onInspectedElement);
+    bridge.addListener('shutdown', onDisconnect);
+    bridge.addListener('pauseElementPolling', onDisconnect);
 
     const timeoutID = setTimeout(onTimeout, TIMEOUT_DELAY);
   });

packages/react-devtools-shared/src/bridge.js

@@ -263,6 +263,9 @@ type FrontendEvents = {
   overrideHookState: [OverrideHookState],
   overrideProps: [OverrideValue],
   overrideState: [OverrideValue],
+
+  resumeElementPolling: [],
+  pauseElementPolling: [],
 };
 
 class Bridge<

packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js

@@ -24,8 +24,8 @@ import {
 import {TreeStateContext} from './TreeContext';
 import {BridgeContext, StoreContext} from '../context';
 import {
-  checkForUpdate,
   inspectElement,
+  startElementUpdatesPolling,
 } from 'react-devtools-shared/src/inspectedElementCache';
 import {
   clearHookNamesCache,
@@ -59,8 +59,6 @@ type Context = {
 export const InspectedElementContext: ReactContext<Context> =
   createContext<Context>(((null: any): Context));
 
-const POLL_INTERVAL = 1000;
-
 export type Props = {
   children: ReactNodeList,
 };
@@ -228,14 +226,21 @@ export function InspectedElementContextController({
   // Periodically poll the selected element for updates.
   useEffect(() => {
     if (element !== null && bridgeIsAlive) {
-      const checkForUpdateWrapper = () => {
-        checkForUpdate({bridge, element, refresh, store});
-        timeoutID = setTimeout(checkForUpdateWrapper, POLL_INTERVAL);
-      };
-      let timeoutID = setTimeout(checkForUpdateWrapper, POLL_INTERVAL);
+      const {abort, pause, resume} = startElementUpdatesPolling({
+        bridge,
+        element,
+        refresh,
+        store,
+      });
+
+      bridge.addListener('resumeElementPolling', resume);
+      bridge.addListener('pauseElementPolling', pause);
 
       return () => {
-        clearTimeout(timeoutID);
+        bridge.removeListener('resumeElementPolling', resume);
+        bridge.removeListener('pauseElementPolling', pause);
+
+        abort();
       };
     }
   }, [

packages/react-devtools-shared/src/errors/ElementPollingCancellationError.js

@@ -0,0 +1,21 @@
+/**
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ *
+ * @flow
+ */
+
+export default class ElementPollingCancellationError extends Error {
+  constructor() {
+    super();
+
+    // Maintains proper stack trace for where our error was thrown (only available on V8)
+    if (Error.captureStackTrace) {
+      Error.captureStackTrace(this, ElementPollingCancellationError);
+    }
+
+    this.name = 'ElementPollingCancellationError';
+  }
+}

packages/react-devtools-shared/src/inspectedElementCache.js

@@ -11,8 +11,9 @@ import {
   unstable_getCacheForType as getCacheForType,
   startTransition,
 } from 'react';
-import Store from './devtools/store';
-import {inspectElement as inspectElementMutableSource} from './inspectedElementMutableSource';
+import Store from 'react-devtools-shared/src/devtools/store';
+import {inspectElement as inspectElementMutableSource} from 'react-devtools-shared/src/inspectedElementMutableSource';
+import ElementPollingCancellationError from 'react-devtools-shared/src//errors/ElementPollingCancellationError';
 
 import type {FrontendBridge} from 'react-devtools-shared/src/bridge';
 import type {Wakeable} from 'shared/ReactTypes';
@@ -177,15 +178,15 @@ export function checkForUpdate({
   element: Element,
   refresh: RefreshFunction,
   store: Store,
-}): void {
+}): void | Promise<void> {
   const {id} = element;
   const rendererID = store.getRendererIDForElement(id);
 
   if (rendererID == null) {
     return;
   }
 
-  inspectElementMutableSource({
+  return inspectElementMutableSource({
     bridge,
     element,
     path: null,
@@ -202,15 +203,88 @@ export function checkForUpdate({
         });
       }
     },
-
-    // There isn't much to do about errors in this case,
-    // but we should at least log them so they aren't silent.
-    error => {
-      console.error(error);
-    },
   );
 }
 
+function createPromiseWhichResolvesInOneSecond() {
+  return new Promise(resolve => setTimeout(resolve, 1000));
+}
+
+type PollingStatus = 'idle' | 'running' | 'paused' | 'aborted';
+
+export function startElementUpdatesPolling({
+  bridge,
+  element,
+  refresh,
+  store,
+}: {
+  bridge: FrontendBridge,
+  element: Element,
+  refresh: RefreshFunction,
+  store: Store,
+}): {abort: () => void, pause: () => void, resume: () => void} {
+  let status: PollingStatus = 'idle';
+
+  function abort() {
+    status = 'aborted';
+  }
+
+  function resume() {
+    if (status === 'running' || status === 'aborted') {
+      return;
+    }
+
+    status = 'idle';
+    poll();
+  }
+
+  function pause() {
+    if (status === 'paused' || status === 'aborted') {
+      return;
+    }
+
+    status = 'paused';
+  }
+
+  function poll(): Promise<void> {
+    status = 'running';
+
+    return Promise.allSettled([
+      checkForUpdate({bridge, element, refresh, store}),
+      createPromiseWhichResolvesInOneSecond(),
+    ])
+      .then(([{status: updateStatus, reason}]) => {
+        // There isn't much to do about errors in this case,
+        // but we should at least log them, so they aren't silent.
+        // Log only if polling is still active, we can't handle the case when
+        // request was sent, and then bridge was remounted (for example, when user did navigate to a new page),
+        // but at least we can mark that polling was aborted
+        if (updateStatus === 'rejected' && status !== 'aborted') {
+          // This is expected Promise rejection, no need to log it
+          if (reason instanceof ElementPollingCancellationError) {
+            return;
+          }
+
+          console.error(reason);
+        }
+      })
+      .finally(() => {
+        const shouldContinuePolling =
+          status !== 'aborted' && status !== 'paused';
+
+        status = 'idle';
+
+        if (shouldContinuePolling) {
+          return poll();
+        }
+      });
+  }
+
+  poll();
+
+  return {abort, resume, pause};
+}
+
 export function clearCacheBecauseOfError(refresh: RefreshFunction): void {
   startTransition(() => {
     const map = createMap();