Avoid eagerly detecting whether component is error boundary and separ…

…ate the concepts between forced error states and console errors With this approach, we can get rid of the warning dialog
facebook · Jun 2, 2021 · 958b787 · 958b787
1 parent 36827a4
commit 958b787
Show file tree

Hide file tree

Showing 13 changed files with 113 additions and 189 deletions.
diff --git a/packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap b/packages/react-devtools-shared/src/__tests__/__snapshots__/profilingCache-test.js.snap
@@ -719,7 +719,6 @@ Object {
           0,
           1,
           0,
-          0,
           4,
           2,
           12000,
@@ -730,7 +729,6 @@ Object {
           2,
           2,
           3,
-          0,
           4,
           4,
           0,
@@ -741,7 +739,6 @@ Object {
           2,
           2,
           4,
-          0,
           4,
           5,
           1000,
@@ -752,7 +749,6 @@ Object {
           2,
           2,
           0,
-          0,
           4,
           6,
           1000,
@@ -776,7 +772,6 @@ Object {
           2,
           1,
           2,
-          0,
           4,
           7,
           2000,
@@ -1183,7 +1178,6 @@ Object {
           0,
           1,
           0,
-          0,
           4,
           2,
           11000,
@@ -1194,7 +1188,6 @@ Object {
           2,
           2,
           3,
-          0,
           4,
           4,
           0,
@@ -1205,7 +1198,6 @@ Object {
           2,
           2,
           0,
-          0,
           4,
           5,
           1000,
@@ -1229,7 +1221,6 @@ Object {
           2,
           1,
           2,
-          0,
           4,
           6,
           1000,
@@ -1265,7 +1256,6 @@ Object {
           2,
           1,
           2,
-          0,
           4,
           7,
           2000,
@@ -1465,7 +1455,6 @@ Object {
       2,
       1,
       2,
-      0,
       4,
       12,
       2000,
@@ -1658,7 +1647,6 @@ Object {
       0,
       1,
       0,
-      0,
       4,
       14,
       11000,
@@ -1669,7 +1657,6 @@ Object {
       14,
       2,
       3,
-      0,
       4,
       16,
       0,
@@ -1680,7 +1667,6 @@ Object {
       14,
       2,
       0,
-      0,
       4,
       17,
       1000,
@@ -2050,7 +2036,6 @@ Object {
           2,
           1,
           2,
-          0,
           4,
           12,
           2000,
@@ -2288,7 +2273,6 @@ Object {
           0,
           1,
           0,
-          0,
           4,
           14,
           11000,
@@ -2299,7 +2283,6 @@ Object {
           14,
           2,
           3,
-          0,
           4,
           16,
           0,
@@ -2310,7 +2293,6 @@ Object {
           14,
           2,
           0,
-          0,
           4,
           17,
           1000,
@@ -2490,7 +2472,6 @@ Object {
       0,
       1,
       0,
-      0,
       4,
       2,
       0,
@@ -2987,7 +2968,6 @@ Object {
           0,
           1,
           0,
-          0,
           4,
           2,
           0,
@@ -2998,7 +2978,6 @@ Object {
           0,
           2,
           0,
-          0,
           4,
           3,
           0,
@@ -4197,7 +4176,6 @@ Object {
           0,
           1,
           0,
-          0,
           4,
           2,
           0,
@@ -4208,7 +4186,6 @@ Object {
           2,
           2,
           0,
-          0,
           4,
           3,
           0,
@@ -4219,7 +4196,6 @@ Object {
           2,
           3,
           0,
-          0,
           4,
           4,
           0,
@@ -4230,7 +4206,6 @@ Object {
           4,
           4,
           0,
-          0,
           4,
           5,
           0,
@@ -4241,7 +4216,6 @@ Object {
           2,
           5,
           0,
-          0,
           4,
           6,
           0,
@@ -4252,7 +4226,6 @@ Object {
           6,
           4,
           0,
-          0,
           4,
           7,
           0,

diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js
@@ -2405,6 +2405,14 @@ describe('InspectedElement', () => {
       );
 
       const errorBoundaryID = ((store.getElementIDAtIndex(0): any): number);
+      const inspect = index => {
+        // HACK: Recreate TestRenderer instance so we can inspect different
+        // elements
+        testRendererInstance = TestRenderer.create(null, {
+          unstable_isConcurrent: true,
+        });
+        return inspectElementAtIndex(index);
+      };
       const toggleError = async forceError => {
         await withErrorsOrWarningsIgnored(['ErrorBoundary'], async () => {
           await utils.actAsync(() => {
@@ -2421,21 +2429,38 @@ describe('InspectedElement', () => {
         });
       };
 
-      let inspectedElement = await inspectElementAtIndex(1);
+      // Inspect <ErrorBoundary /> and see that we cannot toggle error state
+      // on error boundary itself
+      let inspectedElement = await inspect(0);
+      expect(inspectedElement.canToggleError).toBe(false);
+      expect(inspectedElement.errorBoundaryID).toBe(null);
+
+      // Inspect <Example />
+      inspectedElement = await inspect(1);
       expect(inspectedElement.canToggleError).toBe(true);
       expect(inspectedElement.isErrored).toBe(false);
+      expect(inspectedElement.errorBoundaryID).toBe(errorBoundaryID);
 
+      // now force error state on <Example />
       await toggleError(true);
 
-      inspectedElement = await inspectElementAtIndex(0);
+      // we are in error state now, <Example /> won't show up
+      expect(store.getElementIDAtIndex(1)).toBe(null);
+
+      // Inpsect <ErrorBoundary /> to toggle off the error state
+      inspectedElement = await inspect(0);
       expect(inspectedElement.canToggleError).toBe(true);
       expect(inspectedElement.isErrored).toBe(true);
+      // its error boundary ID is itself because it's caught the error
+      expect(inspectedElement.errorBoundaryID).toBe(errorBoundaryID);
 
       await toggleError(false);
 
-      inspectedElement = await inspectElementAtIndex(0);
+      // We can now inspect <Example /> with ability to toggle again
+      inspectedElement = await inspect(1);
       expect(inspectedElement.canToggleError).toBe(true);
       expect(inspectedElement.isErrored).toBe(false);
+      expect(inspectedElement.errorBoundaryID).toBe(errorBoundaryID);
     });
   });
 });
diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js
@@ -976,41 +976,6 @@ describe('Store', () => {
     `);
   });
 
-  it('should detect if element is an error boundary', async () => {
-    class ErrorBoundary extends React.Component {
-      state = {hasError: false};
-      static getDerivedStateFromError(error) {
-        return {hasError: true};
-      }
-      render() {
-        return this.props.children;
-      }
-    }
-    class ClassComponent extends React.Component {
-      render() {
-        return null;
-      }
-    }
-    const FunctionalComponent = () => null;
-
-    const App = () => (
-      <React.Fragment>
-        <ErrorBoundary>
-          <ClassComponent />
-          <FunctionalComponent />
-        </ErrorBoundary>
-      </React.Fragment>
-    );
-
-    const container = document.createElement('div');
-
-    act(() => ReactDOM.render(<App />, container));
-
-    expect(store.getElementAtIndex(1).isErrorBoundary).toBe(true);
-    expect(store.getElementAtIndex(2).isErrorBoundary).toBe(false);
-    expect(store.getElementAtIndex(3).isErrorBoundary).toBe(false);
-  });
-
   describe('Lazy', () => {
     async function fakeImport(result) {
       return {default: result};

diff --git a/packages/react-devtools-shared/src/backend/legacy/renderer.js b/packages/react-devtools-shared/src/backend/legacy/renderer.js
@@ -407,8 +407,6 @@ export function attach(
       pushOperation(ownerID);
       pushOperation(displayNameStringID);
       pushOperation(keyStringID);
-      // isErrorBoundary not supported in legacy renderer
-      pushOperation(0);
     }
   }
 
@@ -805,6 +803,7 @@ export function attach(
       // Toggle error boundary did not exist in legacy versions
       canToggleError: false,
       isErrored: false,
+      errorBoundaryID: null,
 
       // Suspense did not exist in legacy versions
       canToggleSuspense: false,