DevTools Store emits errors before throwing

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;
+  }
 }

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));
+    }
+  };
 }

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);

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>
     );