[DevTools] Make Element Inspection Feel Snappy (facebook#30555)

There's two problems. The biggest one is that it turns out that Chrome
is throttling looping timers that we're using both while polling and for
batching bridge traffic. This means that bridge traffic a lot of the
time just slows down to 1 second at a time. No wonder it feels sluggish.
The only solution is to not use timers for this.

Even when it doesn't like in Firefox the batching into 100ms still feels
too sluggish.

The fix I use is to batch using a microtask instead so we can still
batch multiple commands sent in a single event but we never artificially
slow down an interaction.

I don't think we've reevaluated this for a long time since this was in
the initial commit of DevTools to this repo. If it causes other issues
we can follow up on those.

We really shouldn't use timers for debouncing and such. In fact, React
itself recommends against it because we have a better technique with
scheduling in Concurrent Mode. The correct way to implement this in the
bridge is using a form of back-pressure where we don't keep sending
messages until we get a message back and only send the last one that
matters. E.g. when moving the cursor over a the elements tab we
shouldn't let the backend one-by-one move the DOM node to each one we
have ever passed. We should just move to the last one we're currently
hovering over. But this can't be done at the bridge layer since it
doesn't know if it's a last-one-wins or imperative operation where each
one needs to be sent. It needs to be done higher. I'm not currently
seeing any perf problems with this new approach but I'm curious on React
Native or some thing. RN might need the back-pressure approach. That can
be a follow up if we ever find a test case.

Finally, the other problem is that we use a Suspense boundary around the
Element Inspection. Suspense boundaries are for things that are expected
to take a long time to load. This shows a loading state immediately. To
avoid flashing when it ends up being fast, React throttles the reveal to
200ms. This means that we take a minimum of 200ms to show the props. The
way to show fast async data in React is using a Transition (either using
startTransition or useDeferredValue). This lets the old value remaining
in place while we're loading the next one.

We already implement this using `inspectedElementID` which is the async
one. It would be more idiomatic to implement this with useDeferredValue
rather than the reducer we have now but same principle. We were just
using the wrong ID in a few places so when it synchronously updated they
suspended. So I just made them use the inspectedElementID instead.

Then I can simply remove the Suspense boundary. Now the selection
updates in the tree view synchronously and the sidebar lags a frame or
two but it feels instant. It doesn't flash to white between which is
key.

Author: sebmarkbage

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

@@ -117,12 +117,11 @@ describe('InspectedElement', () => {
         <SettingsContextController>
           <TreeContextController
             defaultSelectedElementID={defaultSelectedElementID}
-            defaultSelectedElementIndex={defaultSelectedElementIndex}>
-            <React.Suspense fallback="Loading...">
-              <InspectedElementContextController>
-                {children}
-              </InspectedElementContextController>
-            </React.Suspense>
+            defaultSelectedElementIndex={defaultSelectedElementIndex}
+            defaultInspectedElementID={defaultSelectedElementID}>
+            <InspectedElementContextController>
+              {children}
+            </InspectedElementContextController>
           </TreeContextController>
         </SettingsContextController>
       </StoreContext.Provider>

packages/react-devtools-shared/src/__tests__/setupTests.js

@@ -128,6 +128,11 @@ beforeEach(() => {
   // Fake timers let us flush Bridge operations between setup and assertions.
   jest.useFakeTimers();
 
+  // We use fake timers heavily in tests but the bridge batching now uses microtasks.
+  global.devtoolsJestTestScheduler = callback => {
+    setTimeout(callback, 0);
+  };
+
   // Use utils.js#withErrorsOrWarningsIgnored instead of directly mutating this array.
   global._ignoredErrorOrWarningMessages = [
     'react-test-renderer is deprecated.',

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

@@ -19,8 +19,6 @@ import type {
 } from 'react-devtools-shared/src/backend/types';
 import type {StyleAndLayout as StyleAndLayoutPayload} from 'react-devtools-shared/src/backend/NativeStyleEditor/types';
 
-const BATCH_DURATION = 100;
-
 // This message specifies the version of the DevTools protocol currently supported by the backend,
 // as well as the earliest NPM version (e.g. "4.13.0") that protocol is supported by on the frontend.
 // This enables an older frontend to display an upgrade message to users for a newer, unsupported backend.
@@ -276,7 +274,7 @@ class Bridge<
 }> {
   _isShutdown: boolean = false;
   _messageQueue: Array<any> = [];
-  _timeoutID: TimeoutID | null = null;
+  _scheduledFlush: boolean = false;
   _wall: Wall;
   _wallUnlisten: Function | null = null;
 
@@ -324,8 +322,19 @@ class Bridge<
     //   (or we're waiting for our setTimeout-0 to fire), then _timeoutID will
     //   be set, and we'll simply add to the queue and wait for that
     this._messageQueue.push(event, payload);
-    if (!this._timeoutID) {
-      this._timeoutID = setTimeout(this._flush, 0);
+    if (!this._scheduledFlush) {
+      this._scheduledFlush = true;
+      // $FlowFixMe
+      if (typeof devtoolsJestTestScheduler === 'function') {
+        // This exists just for our own jest tests.
+        // They're written in such a way that we can neither mock queueMicrotask
+        // because then we break React DOM and we can't not mock it because then
+        // we can't synchronously flush it. So they need to be rewritten.
+        // $FlowFixMe
+        devtoolsJestTestScheduler(this._flush); // eslint-disable-line no-undef
+      } else {
+        queueMicrotask(this._flush);
+      }
     }
   }
 
@@ -363,34 +372,23 @@ class Bridge<
     do {
       this._flush();
     } while (this._messageQueue.length);
-
-    // Make sure once again that there is no dangling timer.
-    if (this._timeoutID !== null) {
-      clearTimeout(this._timeoutID);
-      this._timeoutID = null;
-    }
   }
 
   _flush: () => void = () => {
     // This method is used after the bridge is marked as destroyed in shutdown sequence,
     // so we do not bail out if the bridge marked as destroyed.
     // It is a private method that the bridge ensures is only called at the right times.
-
-    if (this._timeoutID !== null) {
-      clearTimeout(this._timeoutID);
-      this._timeoutID = null;
-    }
-
-    if (this._messageQueue.length) {
-      for (let i = 0; i < this._messageQueue.length; i += 2) {
-        this._wall.send(this._messageQueue[i], ...this._messageQueue[i + 1]);
+    try {
+      if (this._messageQueue.length) {
+        for (let i = 0; i < this._messageQueue.length; i += 2) {
+          this._wall.send(this._messageQueue[i], ...this._messageQueue[i + 1]);
+        }
+        this._messageQueue.length = 0;
       }
-      this._messageQueue.length = 0;
-
-      // Check again for queued messages in BATCH_DURATION ms. This will keep
-      // flushing in a loop as long as messages continue to be added. Once no
-      // more are, the timer expires.
-      this._timeoutID = setTimeout(this._flush, BATCH_DURATION);
+    } finally {
+      // We set this at the end in case new messages are added synchronously above.
+      // They're already handled so they shouldn't queue more flushes.
+      this._scheduledFlush = false;
     }
   };
 

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

@@ -8,14 +8,7 @@
  */
 
 import * as React from 'react';
-import {
-  Fragment,
-  Suspense,
-  useEffect,
-  useLayoutEffect,
-  useReducer,
-  useRef,
-} from 'react';
+import {Fragment, useEffect, useLayoutEffect, useReducer, useRef} from 'react';
 import Tree from './Tree';
 import {OwnersListContextController} from './OwnersListContext';
 import portaledContent from '../portaledContent';
@@ -169,11 +162,9 @@ function Components(_: {}) {
             <div className={styles.InspectedElementWrapper}>
               <NativeStyleContextController>
                 <InspectedElementErrorBoundary>
-                  <Suspense fallback={<Loading />}>
-                    <InspectedElementContextController>
-                      <InspectedElement />
-                    </InspectedElementContextController>
-                  </Suspense>
+                  <InspectedElementContextController>
+                    <InspectedElement />
+                  </InspectedElementContextController>
                 </InspectedElementErrorBoundary>
               </NativeStyleContextController>
             </div>
@@ -186,10 +177,6 @@ function Components(_: {}) {
   );
 }
 
-function Loading() {
-  return <div className={styles.Loading}>Loading...</div>;
-}
-
 const LOCAL_STORAGE_KEY = 'React::DevTools::createResizeReducer';
 const VERTICAL_MODE_MAX_WIDTH = 600;
 const MINIMUM_SIZE = 50;

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

@@ -66,7 +66,7 @@ export type Props = {
 export function InspectedElementContextController({
   children,
 }: Props): React.Node {
-  const {selectedElementID} = useContext(TreeStateContext);
+  const {inspectedElementID} = useContext(TreeStateContext);
   const fetchFileWithCaching = useContext(FetchFileWithCachingContext);
   const bridge = useContext(BridgeContext);
   const store = useContext(StoreContext);
@@ -93,7 +93,9 @@ export function InspectedElementContextController({
   });
 
   const element =
-    selectedElementID !== null ? store.getElementByID(selectedElementID) : null;
+    inspectedElementID !== null
+      ? store.getElementByID(inspectedElementID)
+      : null;
 
   const alreadyLoadedHookNames =
     element != null && hasAlreadyLoadedHookNames(element);

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

@@ -27,7 +27,7 @@ export default function InspectedElementErrorBoundaryWrapper({
 }: WrapperProps): React.Node {
   // Key on the selected element ID so that changing the selected element automatically hides the boundary.
   // This seems best since an error inspecting one element isn't likely to be relevant to another element.
-  const {selectedElementID} = useContext(TreeStateContext);
+  const {inspectedElementID} = useContext(TreeStateContext);
 
   const refresh = useCacheRefresh();
   const handleDsmiss = useCallback(() => {
@@ -37,7 +37,7 @@ export default function InspectedElementErrorBoundaryWrapper({
   return (
     <div className={styles.Wrapper}>
       <ErrorBoundary
-        key={selectedElementID}
+        key={inspectedElementID}
         canDismiss={true}
         onBeforeDismissCallback={handleDsmiss}>
         {children}