DevTools Store emits errors before throwing (#21426)

Brian Vaughn · web-flow · commit d19257b8fabd · 2021-05-04T10:46:26.000-04:00
The Store should never throw an Error without also emitting an event. Otherwise Store errors will be invisible to users, but the downstream errors they cause will be reported as bugs. (For example, github.com//issues/21402) Emitting an error event allows the ErrorBoundary to show the original error. Throwing is still valuable for local development and for unit testing the Store itself.
diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js
@@ -78,6 +78,7 @@ export type Capabilities = {|
 export default class Store extends EventEmitter<{|
   collapseNodesByDefault: [],
   componentFilters: [],
+  error: [Error],
   mutated: [[Array<number>, Map<number, number>]],
   recordChangeDescriptions: [],
   roots: [],
@@ -270,12 +271,14 @@ export default class Store extends EventEmitter<{|
   assertMapSizeMatchesRootCount(map: Map<any, any>, mapName: string) {
     const expectedSize = this.roots.length;
     if (map.size !== expectedSize) {
-      throw new Error(
-        `Expected ${mapName} to contain ${expectedSize} items, but it contains ${
-          map.size
-        } items\n\n${inspect(map, {
-          depth: 20,
-        })}`,
+      this._throwAndEmitError(
+        Error(
+          `Expected ${mapName} to contain ${expectedSize} items, but it contains ${
+            map.size
+          } items\n\n${inspect(map, {
+            depth: 20,
+          })}`,
+        ),
       );
     }
   }
@@ -301,7 +304,9 @@ export default class Store extends EventEmitter<{|
     if (this._profilerStore.isProfiling) {
       // Re-mounting a tree while profiling is in progress might break a lot of assumptions.
       // If necessary, we could support this- but it doesn't seem like a necessary use case.
-      throw Error('Cannot modify filter preferences while profiling');
+      this._throwAndEmitError(
+        Error('Cannot modify filter preferences while profiling'),
+      );
     }
 
     // Filter updates are expensive to apply (since they impact the entire tree).
@@ -607,7 +612,7 @@ export default class Store extends EventEmitter<{|
             }
 
             if (depth === 0) {
-              throw Error('Invalid owners list');
+              this._throwAndEmitError(Error('Invalid owners list'));
             }
 
             list.push({...innerElement, depth});
@@ -667,7 +672,7 @@ export default class Store extends EventEmitter<{|
     if (element !== null) {
       if (isCollapsed) {
         if (element.type === ElementTypeRoot) {
-          throw Error('Root nodes cannot be collapsed');
+          this._throwAndEmitError(Error('Root nodes cannot be collapsed'));
         }
 
         if (!element.isCollapsed) {
@@ -825,8 +830,10 @@ export default class Store extends EventEmitter<{|
           i += 3;
 
           if (this._idToElement.has(id)) {
-            throw Error(
-              `Cannot add node "${id}" because a node with that id is already in the Store.`,
+            this._throwAndEmitError(
+              Error(
+                `Cannot add node "${id}" because a node with that id is already in the Store.`,
+              ),
             );
           }
 
@@ -888,8 +895,10 @@ export default class Store extends EventEmitter<{|
             }
 
             if (!this._idToElement.has(parentID)) {
-              throw Error(
-                `Cannot add child "${id}" to parent "${parentID}" because parent node was not found in the Store.`,
+              this._throwAndEmitError(
+                Error(
+                  `Cannot add child "${id}" to parent "${parentID}" because parent node was not found in the Store.`,
+                ),
               );
             }
 
@@ -940,8 +949,10 @@ export default class Store extends EventEmitter<{|
             const id = ((operations[i]: any): number);
 
             if (!this._idToElement.has(id)) {
-              throw Error(
-                `Cannot remove node "${id}" because no matching node was found in the Store.`,
+              this._throwAndEmitError(
+                Error(
+                  `Cannot remove node "${id}" because no matching node was found in the Store.`,
+                ),
               );
             }
 
@@ -950,7 +961,9 @@ export default class Store extends EventEmitter<{|
             const element = ((this._idToElement.get(id): any): Element);
             const {children, ownerID, parentID, weight} = element;
             if (children.length > 0) {
-              throw new Error(`Node "${id}" was removed before its children.`);
+              this._throwAndEmitError(
+                Error(`Node "${id}" was removed before its children.`),
+              );
             }
 
             this._idToElement.delete(id);
@@ -972,8 +985,10 @@ export default class Store extends EventEmitter<{|
               }
               parentElement = ((this._idToElement.get(parentID): any): Element);
               if (parentElement === undefined) {
-                throw Error(
-                  `Cannot remove node "${id}" from parent "${parentID}" because no matching node was found in the Store.`,
+                this._throwAndEmitError(
+                  Error(
+                    `Cannot remove node "${id}" from parent "${parentID}" because no matching node was found in the Store.`,
+                  ),
                 );
               }
               const index = parentElement.children.indexOf(id);
@@ -1033,16 +1048,20 @@ export default class Store extends EventEmitter<{|
           i += 3;
 
           if (!this._idToElement.has(id)) {
-            throw Error(
-              `Cannot reorder children for node "${id}" because no matching node was found in the Store.`,
+            this._throwAndEmitError(
+              Error(
+                `Cannot reorder children for node "${id}" because no matching node was found in the Store.`,
+              ),
             );
           }
 
           const element = ((this._idToElement.get(id): any): Element);
           const children = element.children;
           if (children.length !== numChildren) {
-            throw Error(
-              `Children cannot be added or removed during a reorder operation.`,
+            this._throwAndEmitError(
+              Error(
+                `Children cannot be added or removed during a reorder operation.`,
+              ),
             );
           }
 
@@ -1087,7 +1106,9 @@ export default class Store extends EventEmitter<{|
           haveErrorsOrWarningsChanged = true;
           break;
         default:
-          throw Error(`Unsupported Bridge operation "${operation}"`);
+          this._throwAndEmitError(
+            Error(`Unsupported Bridge operation "${operation}"`),
+          );
       }
     }
 
@@ -1251,4 +1272,17 @@ export default class Store extends EventEmitter<{|
 
     this.emit('unsupportedBridgeProtocolDetected');
   };
+
+  // The Store should never throw an Error without also emitting an event.
+  // Otherwise Store errors will be invisible to users,
+  // but the downstream errors they cause will be reported as bugs.
+  // For example, https://github.com/facebook/react/issues/21402
+  // Emitting an error event allows the ErrorBoundary to show the original error.
+  _throwAndEmitError(error: Error) {
+    this.emit('error', error);
+
+    // Throwing is still valuable for local development
+    // and for unit testing the Store itself.
+    throw error;
+  }
 }
diff --git a/packages/react-devtools-shared/src/devtools/views/ErrorBoundary/ErrorBoundary.js b/packages/react-devtools-shared/src/devtools/views/ErrorBoundary/ErrorBoundary.js
@@ -9,12 +9,14 @@
 
 import * as React from 'react';
 import {Component, Suspense} from 'react';
+import Store from 'react-devtools-shared/src/devtools/store';
 import ErrorView from './ErrorView';
 import SearchingGitHubIssues from './SearchingGitHubIssues';
 import SuspendingErrorView from './SuspendingErrorView';
 
 type Props = {|
   children: React$Node,
+  store: Store,
 |};
 
 type State = {|
@@ -42,13 +44,6 @@ export default class ErrorBoundary extends Component<Props, State> {
         ? error.message
         : '' + error;
 
-    return {
-      errorMessage,
-      hasError: true,
-    };
-  }
-
-  componentDidCatch(error: any, {componentStack}: any) {
     const callStack =
       typeof error === 'object' &&
       error !== null &&
@@ -59,12 +54,27 @@ export default class ErrorBoundary extends Component<Props, State> {
             .join('\n')
         : null;
 
-    this.setState({
+    return {
       callStack,
+      errorMessage,
+      hasError: true,
+    };
+  }
+
+  componentDidCatch(error: any, {componentStack}: any) {
+    this.setState({
       componentStack,
     });
   }
 
+  componentDidMount() {
+    this.props.store.addListener('error', this._onStoreError);
+  }
+
+  componentWillUnmount() {
+    this.props.store.removeListener('error', this._onStoreError);
+  }
+
   render() {
     const {children} = this.props;
     const {callStack, componentStack, errorMessage, hasError} = this.state;
@@ -88,4 +98,10 @@ export default class ErrorBoundary extends Component<Props, State> {
 
     return children;
   }
+
+  _onStoreError = (error: Error) => {
+    if (!this.state.hasError) {
+      this.setState(ErrorBoundary.getDerivedStateFromError(error));
+    }
+  };
 }
diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/Profiler.js b/packages/react-devtools-shared/src/devtools/views/Profiler/Profiler.js
@@ -26,7 +26,6 @@ import SettingsModal from 'react-devtools-shared/src/devtools/views/Settings/Set
 import SettingsModalContextToggle from 'react-devtools-shared/src/devtools/views/Settings/SettingsModalContextToggle';
 import {SettingsModalContextController} from 'react-devtools-shared/src/devtools/views/Settings/SettingsModalContext';
 import portaledContent from '../portaledContent';
-import Store from '../../store';
 
 import styles from './Profiler.css';
 
@@ -188,10 +187,4 @@ const RecordingInProgress = () => (
   </div>
 );
 
-function onErrorRetry(store: Store) {
-  // If an error happened in the Profiler,
-  // we should clear data on retry (or it will just happen again).
-  store.profilerStore.profilingData = null;
-}
-
-export default portaledContent(Profiler, onErrorRetry);
+export default portaledContent(Profiler);
diff --git a/packages/react-devtools-shared/src/devtools/views/portaledContent.js b/packages/react-devtools-shared/src/devtools/views/portaledContent.js
@@ -12,19 +12,17 @@ import {useContext} from 'react';
 import {createPortal} from 'react-dom';
 import ErrorBoundary from './ErrorBoundary';
 import {StoreContext} from './context';
-import Store from '../store';
 
 export type Props = {portalContainer?: Element, ...};
 
 export default function portaledContent(
   Component: React$StatelessFunctionalComponent<any>,
-  onErrorRetry?: (store: Store) => void,
 ): React$StatelessFunctionalComponent<any> {
   return function PortaledContent({portalContainer, ...rest}: Props) {
     const store = useContext(StoreContext);
 
     const children = (
-      <ErrorBoundary store={store} onRetry={onErrorRetry}>
+      <ErrorBoundary store={store}>
         <Component {...rest} />
       </ErrorBoundary>
     );
