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.
Brian Vaughn committed
May 4, 2021 at 10:46 UTC
d19257b8fabd883d1db91811d81e10651d059e71
4 files changed
+83
-42
packages/react-devtools-shared/src/devtools/store.js
+57
-23
@@ -78,6 +78,7 @@ export type Capabilities = {|
78
export default class Store extends EventEmitter<{|
79
collapseNodesByDefault: [],
80
componentFilters: [],
81
+ error: [Error],
82
mutated: [[Array<number>, Map<number, number>]],
83
recordChangeDescriptions: [],
84
roots: [],
@@ -270,12 +271,14 @@ export default class Store extends EventEmitter<{|
271
assertMapSizeMatchesRootCount(map: Map<any, any>, mapName: string) {
272
const expectedSize = this.roots.length;
273
if (map.size !== expectedSize) {
273
- throw new Error(
274
- `Expected ${mapName} to contain ${expectedSize} items, but it contains ${
275
- map.size
276
- } items\n\n${inspect(map, {
277
- depth: 20,
278
- })}`,
274
+ this._throwAndEmitError(
275
+ Error(
276
+ `Expected ${mapName} to contain ${expectedSize} items, but it contains ${
277
+ map.size
278
+ } items\n\n${inspect(map, {
279
+ depth: 20,
280
+ })}`,
281
+ ),
282
);
283
}
284
}
@@ -301,7 +304,9 @@ export default class Store extends EventEmitter<{|
304
if (this._profilerStore.isProfiling) {
305
// Re-mounting a tree while profiling is in progress might break a lot of assumptions.
306
// If necessary, we could support this- but it doesn't seem like a necessary use case.
304
- throw Error('Cannot modify filter preferences while profiling');
307
+ this._throwAndEmitError(
308
+ Error('Cannot modify filter preferences while profiling'),
309
+ );
310
}
311
312
// Filter updates are expensive to apply (since they impact the entire tree).
@@ -607,7 +612,7 @@ export default class Store extends EventEmitter<{|
612
}
613
614
if (depth === 0) {
610
- throw Error('Invalid owners list');
615
+ this._throwAndEmitError(Error('Invalid owners list'));
616
}
617
618
list.push({...innerElement, depth});
@@ -667,7 +672,7 @@ export default class Store extends EventEmitter<{|
672
if (element !== null) {
673
if (isCollapsed) {
674
if (element.type === ElementTypeRoot) {
670
- throw Error('Root nodes cannot be collapsed');
675
+ this._throwAndEmitError(Error('Root nodes cannot be collapsed'));
676
}
677
678
if (!element.isCollapsed) {
@@ -825,8 +830,10 @@ export default class Store extends EventEmitter<{|
830
i += 3;
831
832
if (this._idToElement.has(id)) {
828
- throw Error(
829
- `Cannot add node "${id}" because a node with that id is already in the Store.`,
833
+ this._throwAndEmitError(
834
+ Error(
835
+ `Cannot add node "${id}" because a node with that id is already in the Store.`,
836
+ ),
837
);
838
}
839
@@ -888,8 +895,10 @@ export default class Store extends EventEmitter<{|
895
}
896
897
if (!this._idToElement.has(parentID)) {
891
- throw Error(
892
- `Cannot add child "${id}" to parent "${parentID}" because parent node was not found in the Store.`,
898
+ this._throwAndEmitError(
899
+ Error(
900
+ `Cannot add child "${id}" to parent "${parentID}" because parent node was not found in the Store.`,
901
+ ),
902
);
903
}
904
@@ -940,8 +949,10 @@ export default class Store extends EventEmitter<{|
949
const id = ((operations[i]: any): number);
950
951
if (!this._idToElement.has(id)) {
943
- throw Error(
944
- `Cannot remove node "${id}" because no matching node was found in the Store.`,
952
+ this._throwAndEmitError(
953
+ Error(
954
+ `Cannot remove node "${id}" because no matching node was found in the Store.`,
955
+ ),
956
);
957
}
958
@@ -950,7 +961,9 @@ export default class Store extends EventEmitter<{|
961
const element = ((this._idToElement.get(id): any): Element);
962
const {children, ownerID, parentID, weight} = element;
963
if (children.length > 0) {
953
- throw new Error(`Node "${id}" was removed before its children.`);
964
+ this._throwAndEmitError(
965
+ Error(`Node "${id}" was removed before its children.`),
966
+ );
967
}
968
969
this._idToElement.delete(id);
@@ -972,8 +985,10 @@ export default class Store extends EventEmitter<{|
985
}
986
parentElement = ((this._idToElement.get(parentID): any): Element);
987
if (parentElement === undefined) {
975
- throw Error(
976
- `Cannot remove node "${id}" from parent "${parentID}" because no matching node was found in the Store.`,
988
+ this._throwAndEmitError(
989
+ Error(
990
+ `Cannot remove node "${id}" from parent "${parentID}" because no matching node was found in the Store.`,
991
+ ),
992
);
993
}
994
const index = parentElement.children.indexOf(id);
@@ -1033,16 +1048,20 @@ export default class Store extends EventEmitter<{|
1048
i += 3;
1049
1050
if (!this._idToElement.has(id)) {
1036
- throw Error(
1037
- `Cannot reorder children for node "${id}" because no matching node was found in the Store.`,
1051
+ this._throwAndEmitError(
1052
+ Error(
1053
+ `Cannot reorder children for node "${id}" because no matching node was found in the Store.`,
1054
+ ),
1055
);
1056
}
1057
1058
const element = ((this._idToElement.get(id): any): Element);
1059
const children = element.children;
1060
if (children.length !== numChildren) {
1044
- throw Error(
1045
- `Children cannot be added or removed during a reorder operation.`,
1061
+ this._throwAndEmitError(
1062
+ Error(
1063
+ `Children cannot be added or removed during a reorder operation.`,
1064
+ ),
1065
);
1066
}
1067
@@ -1087,7 +1106,9 @@ export default class Store extends EventEmitter<{|
1106
haveErrorsOrWarningsChanged = true;
1107
break;
1108
default:
1090
- throw Error(`Unsupported Bridge operation "${operation}"`);
1109
+ this._throwAndEmitError(
1110
+ Error(`Unsupported Bridge operation "${operation}"`),
1111
+ );
1112
}
1113
}
1114
@@ -1251,4 +1272,17 @@ export default class Store extends EventEmitter<{|
1272
1273
this.emit('unsupportedBridgeProtocolDetected');
1274
};
1275
+
1276
+ // The Store should never throw an Error without also emitting an event.
1277
+ // Otherwise Store errors will be invisible to users,
1278
+ // but the downstream errors they cause will be reported as bugs.
1279
+ // For example, https://github.com/facebook/react/issues/21402
1280
+ // Emitting an error event allows the ErrorBoundary to show the original error.
1281
+ _throwAndEmitError(error: Error) {
1282
+ this.emit('error', error);
1283
+
1284
+ // Throwing is still valuable for local development
1285
+ // and for unit testing the Store itself.
1286
+ throw error;
1287
+ }
1288
}
packages/react-devtools-shared/src/devtools/views/ErrorBoundary/ErrorBoundary.js
+24
-8
@@ -9,12 +9,14 @@
9
10
import * as React from 'react';
11
import {Component, Suspense} from 'react';
12
+import Store from 'react-devtools-shared/src/devtools/store';
13
import ErrorView from './ErrorView';
14
import SearchingGitHubIssues from './SearchingGitHubIssues';
15
import SuspendingErrorView from './SuspendingErrorView';
16
17
type Props = {|
18
children: React$Node,
19
+ store: Store,
20
|};
21
22
type State = {|
@@ -42,13 +44,6 @@ export default class ErrorBoundary extends Component<Props, State> {
44
? error.message
45
: '' + error;
46
45
- return {
46
- errorMessage,
47
- hasError: true,
48
- };
49
- }
50
-
51
- componentDidCatch(error: any, {componentStack}: any) {
47
const callStack =
48
typeof error === 'object' &&
49
error !== null &&
@@ -59,12 +54,27 @@ export default class ErrorBoundary extends Component<Props, State> {
54
.join('\n')
55
: null;
56
62
- this.setState({
57
+ return {
58
callStack,
59
+ errorMessage,
60
+ hasError: true,
61
+ };
62
+ }
63
+
64
+ componentDidCatch(error: any, {componentStack}: any) {
65
+ this.setState({
66
componentStack,
67
});
68
}
69
70
+ componentDidMount() {
71
+ this.props.store.addListener('error', this._onStoreError);
72
+ }
73
+
74
+ componentWillUnmount() {
75
+ this.props.store.removeListener('error', this._onStoreError);
76
+ }
77
+
78
render() {
79
const {children} = this.props;
80
const {callStack, componentStack, errorMessage, hasError} = this.state;
@@ -88,4 +98,10 @@ export default class ErrorBoundary extends Component<Props, State> {
98
99
return children;
100
}
101
+
102
+ _onStoreError = (error: Error) => {
103
+ if (!this.state.hasError) {
104
+ this.setState(ErrorBoundary.getDerivedStateFromError(error));
105
+ }
106
+ };
107
}
packages/react-devtools-shared/src/devtools/views/Profiler/Profiler.js
+1
-8
@@ -26,7 +26,6 @@ import SettingsModal from 'react-devtools-shared/src/devtools/views/Settings/Set
26
import SettingsModalContextToggle from 'react-devtools-shared/src/devtools/views/Settings/SettingsModalContextToggle';
27
import {SettingsModalContextController} from 'react-devtools-shared/src/devtools/views/Settings/SettingsModalContext';
28
import portaledContent from '../portaledContent';
29
-import Store from '../../store';
29
30
import styles from './Profiler.css';
31
@@ -188,10 +187,4 @@ const RecordingInProgress = () => (
187
</div>
188
);
189
191
-function onErrorRetry(store: Store) {
192
- // If an error happened in the Profiler,
193
- // we should clear data on retry (or it will just happen again).
194
- store.profilerStore.profilingData = null;
195
-}
196
-
197
-export default portaledContent(Profiler, onErrorRetry);
190
+export default portaledContent(Profiler);
packages/react-devtools-shared/src/devtools/views/portaledContent.js
+1
-3
@@ -12,19 +12,17 @@ import {useContext} from 'react';
12
import {createPortal} from 'react-dom';
13
import ErrorBoundary from './ErrorBoundary';
14
import {StoreContext} from './context';
15
-import Store from '../store';
15
16
export type Props = {portalContainer?: Element, ...};
17
18
export default function portaledContent(
19
Component: React$StatelessFunctionalComponent<any>,
21
- onErrorRetry?: (store: Store) => void,
20
): React$StatelessFunctionalComponent<any> {
21
return function PortaledContent({portalContainer, ...rest}: Props) {
22
const store = useContext(StoreContext);
23
24
const children = (
27
- <ErrorBoundary store={store} onRetry={onErrorRetry}>
25
+ <ErrorBoundary store={store}>
26
<Component {...rest} />
27
</ErrorBoundary>
28
);