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 = ( - + );