From 09d10e60ef000b225ecac14c14df96852986058c Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 4 May 2021 10:46:26 -0400 Subject: [PATCH] DevTools Store emits errors before throwing (#21426) 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/facebook/react/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. --- .../src/devtools/store.js | 80 +++++++++++++------ .../views/ErrorBoundary/ErrorBoundary.js | 32 ++++++-- .../src/devtools/views/Profiler/Profiler.js | 9 +-- .../src/devtools/views/portaledContent.js | 4 +- 4 files changed, 83 insertions(+), 42 deletions(-) diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index ff3cf487b9b9d..0d2ca5faa88ed 100644 --- 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, Map]], recordChangeDescriptions: [], roots: [], @@ -270,12 +271,14 @@ export default class Store extends EventEmitter<{| assertMapSizeMatchesRootCount(map: Map, 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 index 3d633cf5dca29..45441a925df65 100644 --- 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 { ? 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 { .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 { 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 index 58008334bc8f2..9fda41499871e 100644 --- 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 = () => ( ); -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 index f0996702c280a..4d7cdde32c951 100644 --- 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, - onErrorRetry?: (store: Store) => void, ): React$StatelessFunctionalComponent { return function PortaledContent({portalContainer, ...rest}: Props) { const store = useContext(StoreContext); const children = ( - + );