Skip to content

Commit

Permalink
DevTools Store emits errors before throwing (facebook#21426)
Browse files Browse the repository at this point in the history 
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/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.
  • Loading branch information
@koto
Brian Vaughn authored and koto committed Jun 15, 2021
1 parent 1045d46 commit 09d10e6
Show file tree
Hide file tree
Showing 4 changed files with 83 additions and 42 deletions.
80 changes: 57 additions & 23 deletions packages/react-devtools-shared/src/devtools/store.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: [],
Expand Down Expand Up @@ -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,
})}`,
),
);
}
}
Expand All @@ -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).
Expand Down Expand Up @@ -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});
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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.`,
),
);
}

Expand Down Expand Up @@ -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.`,
),
);
}

Expand Down Expand Up @@ -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.`,
),
);
}

Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -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.`,
),
);
}

Expand Down Expand Up @@ -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}"`),
);
}
}

Expand Down Expand Up @@ -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;
}
}
32 changes: 24 additions & 8 deletions packages/react-devtools-shared/src/devtools/views/ErrorBoundary/ErrorBoundary.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 = {|
Expand Down Expand Up @@ -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 &&
Expand All @@ -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;
Expand All @@ -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));
}
};
}
9 changes: 1 addition & 8 deletions packages/react-devtools-shared/src/devtools/views/Profiler/Profiler.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand Down Expand Up @@ -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);
4 changes: 1 addition & 3 deletions packages/react-devtools-shared/src/devtools/views/portaledContent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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>
);
Expand Down

0 comments on commit 09d10e6

Please sign in to comment.