refactor: data source for errors and warnings tracking is now in Store (#31010)
Stacked on https://github.com/facebook/react/pull/31009. 1. Instead of keeping `showInlineWarningsAndErrors` in `Settings` context (which was removed in https://github.com/facebook/react/pull/30610), `Store` will now have a boolean flag, which controls if the UI should be displaying information about errors and warnings. 2. The errors and warnings counters in the Tree view are now counting only unique errors. This makes more sense, because it is part of the Elements Tree view, so ideally it should be showing number of components with errors and number of components of warnings. Consider this example: 2.1. Warning for element `A` was emitted once and warning for element `B` was emitted twice. 2.2. With previous implementation, we would show `3 ⚠️`, because in total there were 3 warnings in total. If user tries to iterate through these, it will only take 2 steps to do the full cycle, because there are only 2 elements with warnings (with one having same warning, which was emitted twice). 2.3 With current implementation, we would show `2 ⚠️`. Inspecting the element with doubled warning will still show the warning counter (2) before the warning message. With these changes, the feature correctly works. https://fburl.com/a7fw92m4
Ruslan Lesiutin committed
Sep 24, 2024 at 19:51 UTC
a15bbe14751287cb7ac124ff88f694d0883f3ac6
8 files changed
+109
-83
packages/react-devtools-shared/src/__tests__/store-test.js
+2
-2
@@ -2148,8 +2148,8 @@ describe('Store', () => {
2148
act(() => render(<React.Fragment />));
2149
});
2150
expect(store).toMatchInlineSnapshot(`[root]`);
2151
- expect(store.errorCount).toBe(0);
2152
- expect(store.warningCount).toBe(0);
2151
+ expect(store.componentWithErrorCount).toBe(0);
2152
+ expect(store.componentWithWarningCount).toBe(0);
2153
});
2154
2155
// Regression test for https://github.com/facebook/react/issues/23202
packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js
+8
-8
@@ -420,8 +420,8 @@ describe('Store component filters', () => {
420
});
421
422
expect(store).toMatchInlineSnapshot(``);
423
- expect(store.errorCount).toBe(0);
424
- expect(store.warningCount).toBe(0);
423
+ expect(store.componentWithErrorCount).toBe(0);
424
+ expect(store.componentWithWarningCount).toBe(0);
425
426
await actAsync(async () => (store.componentFilters = []));
427
expect(store).toMatchInlineSnapshot(`
@@ -460,8 +460,8 @@ describe('Store component filters', () => {
460
]),
461
);
462
expect(store).toMatchInlineSnapshot(`[root]`);
463
- expect(store.errorCount).toBe(0);
464
- expect(store.warningCount).toBe(0);
463
+ expect(store.componentWithErrorCount).toBe(0);
464
+ expect(store.componentWithWarningCount).toBe(0);
465
466
await actAsync(async () => (store.componentFilters = []));
467
expect(store).toMatchInlineSnapshot(`
@@ -510,8 +510,8 @@ describe('Store component filters', () => {
510
});
511
512
expect(store).toMatchInlineSnapshot(``);
513
- expect(store.errorCount).toBe(0);
514
- expect(store.warningCount).toBe(0);
513
+ expect(store.componentWithErrorCount).toBe(0);
514
+ expect(store.componentWithWarningCount).toBe(0);
515
516
await actAsync(async () => (store.componentFilters = []));
517
expect(store).toMatchInlineSnapshot(`
@@ -550,8 +550,8 @@ describe('Store component filters', () => {
550
]),
551
);
552
expect(store).toMatchInlineSnapshot(`[root]`);
553
- expect(store.errorCount).toBe(0);
554
- expect(store.warningCount).toBe(0);
553
+ expect(store.componentWithErrorCount).toBe(0);
554
+ expect(store.componentWithWarningCount).toBe(0);
555
556
await actAsync(async () => (store.componentFilters = []));
557
expect(store).toMatchInlineSnapshot(`
packages/react-devtools-shared/src/devtools/store.js
+53
-15
@@ -114,8 +114,8 @@ export default class Store extends EventEmitter<{
114
_bridge: FrontendBridge;
115
116
// Computed whenever _errorsAndWarnings Map changes.
117
- _cachedErrorCount: number = 0;
118
- _cachedWarningCount: number = 0;
117
+ _cachedComponentWithErrorCount: number = 0;
118
+ _cachedComponentWithWarningCount: number = 0;
119
_cachedErrorAndWarningTuples: ErrorAndWarningTuples | null = null;
120
121
// Should new nodes be collapsed by default when added to the tree?
@@ -196,6 +196,7 @@ export default class Store extends EventEmitter<{
196
197
_shouldCheckBridgeProtocolCompatibility: boolean = false;
198
_hookSettings: $ReadOnly<DevToolsHookSettings> | null = null;
199
+ _shouldShowWarningsAndErrors: boolean = false;
200
201
constructor(bridge: FrontendBridge, config?: Config) {
202
super();
@@ -383,8 +384,24 @@ export default class Store extends EventEmitter<{
384
return this._bridgeProtocol;
385
}
386
386
- get errorCount(): number {
387
- return this._cachedErrorCount;
387
+ get componentWithErrorCount(): number {
388
+ if (!this._shouldShowWarningsAndErrors) {
389
+ return 0;
390
+ }
391
+
392
+ return this._cachedComponentWithErrorCount;
393
+ }
394
+
395
+ get componentWithWarningCount(): number {
396
+ if (!this._shouldShowWarningsAndErrors) {
397
+ return 0;
398
+ }
399
+
400
+ return this._cachedComponentWithWarningCount;
401
+ }
402
+
403
+ get displayingErrorsAndWarningsEnabled(): boolean {
404
+ return this._shouldShowWarningsAndErrors;
405
}
406
407
get hasOwnerMetadata(): boolean {
@@ -480,10 +497,6 @@ export default class Store extends EventEmitter<{
497
return this._unsupportedRendererVersionDetected;
498
}
499
483
- get warningCount(): number {
484
- return this._cachedWarningCount;
485
- }
486
-
500
containsElement(id: number): boolean {
501
return this._idToElement.has(id);
502
}
@@ -581,7 +594,11 @@ export default class Store extends EventEmitter<{
594
}
595
596
// Returns a tuple of [id, index]
584
- getElementsWithErrorsAndWarnings(): Array<{id: number, index: number}> {
597
+ getElementsWithErrorsAndWarnings(): ErrorAndWarningTuples {
598
+ if (!this._shouldShowWarningsAndErrors) {
599
+ return [];
600
+ }
601
+
602
if (this._cachedErrorAndWarningTuples !== null) {
603
return this._cachedErrorAndWarningTuples;
604
}
@@ -615,6 +632,10 @@ export default class Store extends EventEmitter<{
632
errorCount: number,
633
warningCount: number,
634
} {
635
+ if (!this._shouldShowWarningsAndErrors) {
636
+ return {errorCount: 0, warningCount: 0};
637
+ }
638
+
639
return this._errorsAndWarnings.get(id) || {errorCount: 0, warningCount: 0};
640
}
641
@@ -1325,16 +1346,21 @@ export default class Store extends EventEmitter<{
1346
this._cachedErrorAndWarningTuples = null;
1347
1348
if (haveErrorsOrWarningsChanged) {
1328
- let errorCount = 0;
1329
- let warningCount = 0;
1349
+ let componentWithErrorCount = 0;
1350
+ let componentWithWarningCount = 0;
1351
1352
this._errorsAndWarnings.forEach(entry => {
1332
- errorCount += entry.errorCount;
1333
- warningCount += entry.warningCount;
1353
+ if (entry.errorCount > 0) {
1354
+ componentWithErrorCount++;
1355
+ }
1356
+
1357
+ if (entry.warningCount > 0) {
1358
+ componentWithWarningCount++;
1359
+ }
1360
});
1361
1336
- this._cachedErrorCount = errorCount;
1337
- this._cachedWarningCount = warningCount;
1362
+ this._cachedComponentWithErrorCount = componentWithErrorCount;
1363
+ this._cachedComponentWithWarningCount = componentWithWarningCount;
1364
}
1365
1366
if (haveRootsChanged) {
@@ -1528,9 +1554,21 @@ export default class Store extends EventEmitter<{
1554
onHookSettings: (settings: $ReadOnly<DevToolsHookSettings>) => void =
1555
settings => {
1556
this._hookSettings = settings;
1557
+
1558
+ this.setShouldShowWarningsAndErrors(settings.showInlineWarningsAndErrors);
1559
this.emit('hookSettings', settings);
1560
};
1561
1562
+ setShouldShowWarningsAndErrors(status: boolean): void {
1563
+ const previousStatus = this._shouldShowWarningsAndErrors;
1564
+ this._shouldShowWarningsAndErrors = status;
1565
+
1566
+ if (previousStatus !== status) {
1567
+ // Propagate to subscribers, although tree state has not changed
1568
+ this.emit('mutated', [[], new Map()]);
1569
+ }
1570
+ }
1571
+
1572
// The Store should never throw an Error without also emitting an event.
1573
// Otherwise Store errors will be invisible to users,
1574
// but the downstream errors they cause will be reported as bugs.
packages/react-devtools-shared/src/devtools/views/Components/Element.js
+2
-4
@@ -12,7 +12,6 @@ import {Fragment, useContext, useMemo, useState} from 'react';
12
import Store from 'react-devtools-shared/src/devtools/store';
13
import ButtonIcon from '../ButtonIcon';
14
import {TreeDispatcherContext, TreeStateContext} from './TreeContext';
15
-import {SettingsContext} from '../Settings/SettingsContext';
15
import {StoreContext} from '../context';
16
import {useSubscription} from '../hooks';
17
import {logEvent} from 'react-devtools-shared/src/Logger';
@@ -37,7 +36,6 @@ export default function Element({data, index, style}: Props): React.Node {
36
const {ownerFlatTree, ownerID, selectedElementID} =
37
useContext(TreeStateContext);
38
const dispatch = useContext(TreeDispatcherContext);
40
- const {showInlineWarningsAndErrors} = React.useContext(SettingsContext);
39
40
const element =
41
ownerFlatTree !== null
@@ -181,7 +179,7 @@ export default function Element({data, index, style}: Props): React.Node {
179
className={styles.BadgesBlock}
180
/>
181
184
- {showInlineWarningsAndErrors && errorCount > 0 && (
182
+ {errorCount > 0 && (
183
<Icon
184
type="error"
185
className={
@@ -191,7 +189,7 @@ export default function Element({data, index, style}: Props): React.Node {
189
}
190
/>
191
)}
194
- {showInlineWarningsAndErrors && warningCount > 0 && (
192
+ {warningCount > 0 && (
193
<Icon
194
type="warning"
195
className={
packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorsAndWarningsTree.js
+4
-4
@@ -9,7 +9,6 @@
9
10
import * as React from 'react';
11
import {
12
- useContext,
12
unstable_useCacheRefresh as useCacheRefresh,
13
useTransition,
14
} from 'react';
@@ -18,7 +17,6 @@ import ButtonIcon from '../ButtonIcon';
17
import Store from '../../store';
18
import sharedStyles from './InspectedElementSharedStyles.css';
19
import styles from './InspectedElementErrorsAndWarningsTree.css';
21
-import {SettingsContext} from '../Settings/SettingsContext';
20
import {
21
clearErrorsForElement as clearErrorsForElementAPI,
22
clearWarningsForElement as clearWarningsForElementAPI,
@@ -74,12 +72,14 @@ export default function InspectedElementErrorsAndWarningsTree({
72
}
73
};
74
77
- const {showInlineWarningsAndErrors} = useContext(SettingsContext);
78
- if (!showInlineWarningsAndErrors) {
75
+ if (!store.displayingErrorsAndWarningsEnabled) {
76
return null;
77
}
78
79
const {errors, warnings} = inspectedElement;
80
+ if (errors.length === 0 && warnings.length === 0) {
81
+ return null;
82
+ }
83
84
return (
85
<React.Fragment>
packages/react-devtools-shared/src/devtools/views/Components/Tree.js
+35
-37
@@ -73,7 +73,7 @@ export default function Tree(props: Props): React.Node {
73
74
const [treeFocused, setTreeFocused] = useState<boolean>(false);
75
76
- const {lineHeight, showInlineWarningsAndErrors} = useContext(SettingsContext);
76
+ const {lineHeight} = useContext(SettingsContext);
77
78
// Make sure a newly selected element is visible in the list.
79
// This is helpful for things like the owners list and search.
@@ -325,8 +325,8 @@ export default function Tree(props: Props): React.Node {
325
const errorsOrWarningsSubscription = useMemo(
326
() => ({
327
getCurrentValue: () => ({
328
- errors: store.errorCount,
329
- warnings: store.warningCount,
328
+ errors: store.componentWithErrorCount,
329
+ warnings: store.componentWithWarningCount,
330
}),
331
subscribe: (callback: Function) => {
332
store.addListener('mutated', callback);
@@ -370,40 +370,38 @@ export default function Tree(props: Props): React.Node {
370
<Suspense fallback={<Loading />}>
371
{ownerID !== null ? <OwnersStack /> : <ComponentSearchInput />}
372
</Suspense>
373
- {showInlineWarningsAndErrors &&
374
- ownerID === null &&
375
- (errors > 0 || warnings > 0) && (
376
- <React.Fragment>
377
- <div className={styles.VRule} />
378
- {errors > 0 && (
379
- <div className={styles.IconAndCount}>
380
- <Icon className={styles.ErrorIcon} type="error" />
381
- {errors}
382
- </div>
383
- )}
384
- {warnings > 0 && (
385
- <div className={styles.IconAndCount}>
386
- <Icon className={styles.WarningIcon} type="warning" />
387
- {warnings}
388
- </div>
389
- )}
390
- <Button
391
- onClick={handlePreviousErrorOrWarningClick}
392
- title="Scroll to previous error or warning">
393
- <ButtonIcon type="up" />
394
- </Button>
395
- <Button
396
- onClick={handleNextErrorOrWarningClick}
397
- title="Scroll to next error or warning">
398
- <ButtonIcon type="down" />
399
- </Button>
400
- <Button
401
- onClick={clearErrorsAndWarnings}
402
- title="Clear all errors and warnings">
403
- <ButtonIcon type="clear" />
404
- </Button>
405
- </React.Fragment>
406
- )}
373
+ {ownerID === null && (errors > 0 || warnings > 0) && (
374
+ <React.Fragment>
375
+ <div className={styles.VRule} />
376
+ {errors > 0 && (
377
+ <div className={styles.IconAndCount}>
378
+ <Icon className={styles.ErrorIcon} type="error" />
379
+ {errors}
380
+ </div>
381
+ )}
382
+ {warnings > 0 && (
383
+ <div className={styles.IconAndCount}>
384
+ <Icon className={styles.WarningIcon} type="warning" />
385
+ {warnings}
386
+ </div>
387
+ )}
388
+ <Button
389
+ onClick={handlePreviousErrorOrWarningClick}
390
+ title="Scroll to previous error or warning">
391
+ <ButtonIcon type="up" />
392
+ </Button>
393
+ <Button
394
+ onClick={handleNextErrorOrWarningClick}
395
+ title="Scroll to next error or warning">
396
+ <ButtonIcon type="down" />
397
+ </Button>
398
+ <Button
399
+ onClick={clearErrorsAndWarnings}
400
+ title="Clear all errors and warnings">
401
+ <ButtonIcon type="clear" />
402
+ </Button>
403
+ </React.Fragment>
404
+ )}
405
{!hideSettings && (
406
<Fragment>
407
<div className={styles.VRule} />
packages/react-devtools-shared/src/devtools/views/Settings/DebuggingSettings.js
+4
@@ -37,6 +37,10 @@ export default function DebuggingSettings({
37
const [showInlineWarningsAndErrors, setShowInlineWarningsAndErrors] =
38
useState(usedHookSettings.showInlineWarningsAndErrors);
39
40
+ useEffect(() => {
41
+ store.setShouldShowWarningsAndErrors(showInlineWarningsAndErrors);
42
+ }, [showInlineWarningsAndErrors]);
43
+
44
useEffect(() => {
45
store.updateHookSettings({
46
appendComponentStack,
packages/react-devtools-shared/src/devtools/views/Settings/SettingsContext.js
+1
-13
@@ -43,21 +43,9 @@ type Context = {
43
// Specified as a separate prop so it can trigger a re-render of FixedSizeList.
44
lineHeight: number,
45
46
- appendComponentStack: boolean,
47
- setAppendComponentStack: (value: boolean) => void,
48
-
49
- breakOnConsoleErrors: boolean,
50
- setBreakOnConsoleErrors: (value: boolean) => void,
51
-
46
parseHookNames: boolean,
47
setParseHookNames: (value: boolean) => void,
48
55
- hideConsoleLogsInStrictMode: boolean,
56
- setHideConsoleLogsInStrictMode: (value: boolean) => void,
57
-
58
- showInlineWarningsAndErrors: boolean,
59
- setShowInlineWarningsAndErrors: (value: boolean) => void,
60
-
49
theme: Theme,
50
setTheme(value: Theme): void,
51
@@ -176,7 +164,7 @@ function SettingsContextController({
164
bridge.send('setTraceUpdatesEnabled', traceUpdatesEnabled);
165
}, [bridge, traceUpdatesEnabled]);
166
179
- const value = useMemo(
167
+ const value: Context = useMemo(
168
() => ({
169
displayDensity,
170
lineHeight: