Surface backend errors during inspection in the frontend UI

Previously, if a component were to throw an error while being inspected, the backend would not return a response, causing the frontend to eventually timeout. The fix for this is to catch the error and return it as a new error type response. This allows the frontend to show a more actionable message to the user.
facebook · Oct 12, 2021 · 2f94443 · 2f94443
1 parent 1e247ff
commit 2f94443
Show file tree

Hide file tree

Showing 5 changed files with 98 additions and 13 deletions.
diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js
@@ -35,6 +35,9 @@ describe('InspectedElement', () => {
   let legacyRender;
   let testRendererInstance;
 
+  let ErrorBoundary;
+  let errorBoundaryInstance;
+
   beforeEach(() => {
     utils = require('./utils');
     utils.beforeEachProfiling();
@@ -69,6 +72,23 @@ describe('InspectedElement', () => {
     testRendererInstance = TestRenderer.create(null, {
       unstable_isConcurrent: true,
     });
+
+    errorBoundaryInstance = null;
+
+    ErrorBoundary = class extends React.Component {
+      state = {error: null};
+      componentDidCatch(error) {
+        this.setState({error});
+      }
+      render() {
+        errorBoundaryInstance = this;
+
+        if (this.state.error) {
+          return null;
+        }
+        return this.props.children;
+      }
+    };
   });
 
   afterEach(() => {
@@ -109,7 +129,11 @@ describe('InspectedElement', () => {
 
   function noop() {}
 
-  async function inspectElementAtIndex(index, useCustomHook = noop) {
+  async function inspectElementAtIndex(
+    index,
+    useCustomHook = noop,
+    shouldThrow = false,
+  ) {
     let didFinish = false;
     let inspectedElement = null;
 
@@ -124,17 +148,21 @@ describe('InspectedElement', () => {
 
     await utils.actAsync(() => {
       testRendererInstance.update(
-        <Contexts
-          defaultSelectedElementID={id}
-          defaultSelectedElementIndex={index}>
-          <React.Suspense fallback={null}>
-            <Suspender id={id} index={index} />
-          </React.Suspense>
-        </Contexts>,
+        <ErrorBoundary>
+          <Contexts
+            defaultSelectedElementID={id}
+            defaultSelectedElementIndex={index}>
+            <React.Suspense fallback={null}>
+              <Suspender id={id} index={index} />
+            </React.Suspense>
+          </Contexts>
+        </ErrorBoundary>,
       );
     }, false);
 
-    expect(didFinish).toBe(true);
+    if (!shouldThrow) {
+      expect(didFinish).toBe(true);
+    }
 
     return inspectedElement;
   }
@@ -2069,6 +2097,34 @@ describe('InspectedElement', () => {
     expect(inspectedElement.rootType).toMatchInlineSnapshot(`"createRoot()"`);
   });
 
+  it('should gracefully surface backend errors on the frontend rather than timing out', async () => {
+    spyOn(console, 'error');
+
+    let shouldThrow = false;
+
+    const Example = () => {
+      const [count] = React.useState(0);
+
+      if (shouldThrow) {
+        throw Error('Expected');
+      } else {
+        return count;
+      }
+    };
+
+    await utils.actAsync(() => {
+      const container = document.createElement('div');
+      ReactDOM.createRoot(container).render(<Example />);
+    }, false);
+
+    shouldThrow = true;
+
+    const value = await inspectElementAtIndex(0, noop, true);
+
+    expect(value).toBe(null);
+    expect(errorBoundaryInstance.state.error.message).toBe('Expected');
+  });
+
   describe('$r', () => {
     it('should support function components', async () => {
       const Example = () => {
@@ -2656,7 +2712,7 @@ describe('InspectedElement', () => {
 
   describe('error boundary', () => {
     it('can toggle error', async () => {
-      class ErrorBoundary extends React.Component<any> {
+      class LocalErrorBoundary extends React.Component<any> {
         state = {hasError: false};
         static getDerivedStateFromError(error) {
           return {hasError: true};
@@ -2666,13 +2722,14 @@ describe('InspectedElement', () => {
           return hasError ? 'has-error' : this.props.children;
         }
       }
+
       const Example = () => 'example';
 
       await utils.actAsync(() =>
         legacyRender(
-          <ErrorBoundary>
+          <LocalErrorBoundary>
             <Example />
-          </ErrorBoundary>,
+          </LocalErrorBoundary>,
           document.createElement('div'),
         ),
       );

diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js
@@ -3471,7 +3471,19 @@ export function attach(
 
     hasElementUpdatedSinceLastInspected = false;
 
-    mostRecentlyInspectedElement = inspectElementRaw(id);
+    try {
+      mostRecentlyInspectedElement = inspectElementRaw(id);
+    } catch (error) {
+      console.error('Error inspecting element.\n\n', error);
+
+      return {
+        type: 'error',
+        id,
+        responseID: requestID,
+        value: error.message,
+      };
+    }
+
     if (mostRecentlyInspectedElement === null) {
       return {
         id,

diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js
@@ -267,10 +267,18 @@ export type InspectedElement = {|
   rendererVersion: string | null,
 |};
 
+export const InspectElementErrorType = 'error';
 export const InspectElementFullDataType = 'full-data';
 export const InspectElementNoChangeType = 'no-change';
 export const InspectElementNotFoundType = 'not-found';
 
+export type InspectElementError = {|
+  id: number,
+  responseID: number,
+  type: 'error',
+  value: string,
+|};
+
 export type InspectElementFullData = {|
   id: number,
   responseID: number,
@@ -299,6 +307,7 @@ export type InspectElementNotFound = {|
 |};
 
 export type InspectedElementPayload =
+  | InspectElementError
   | InspectElementFullData
   | InspectElementHydratedPath
   | InspectElementNoChange

diff --git a/packages/react-devtools-shared/src/devtools/views/Components/types.js b/packages/react-devtools-shared/src/devtools/views/Components/types.js
@@ -58,6 +58,7 @@ export type OwnersList = {|
 |};
 
 export type InspectedElementResponseType =
+  | 'error'
   | 'full-data'
   | 'hydrated-path'
   | 'no-change'

diff --git a/packages/react-devtools-shared/src/inspectedElementMutableSource.js b/packages/react-devtools-shared/src/inspectedElementMutableSource.js
@@ -18,6 +18,7 @@ import {fillInPath} from 'react-devtools-shared/src/hydration';
 import type {LRUCache} from 'react-devtools-shared/src/types';
 import type {FrontendBridge} from 'react-devtools-shared/src/bridge';
 import type {
+  InspectElementError,
   InspectElementFullData,
   InspectElementHydratedPath,
 } from 'react-devtools-shared/src/backend/types';
@@ -79,6 +80,11 @@ export function inspectElement({
 
     let inspectedElement;
     switch (type) {
+      case 'error':
+        const error = ((data: any): InspectElementError);
+
+        throw new Error(error.value);
+
       case 'no-change':
         // This is a no-op for the purposes of our cache.
         inspectedElement = inspectedElementCache.get(id);