Skip to content

Commit a15bbe1

Browse files
hoxyqhoxyq
authored
refactor: data source for errors and warnings tracking is now in Store (#31010)
Stacked on #31009. 1. Instead of keeping `showInlineWarningsAndErrors` in `Settings` context (which was removed in #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
1 parent fc4a33e commit a15bbe1

File tree

8 files changed

+109
-83
lines changed

8 files changed

+109
-83
lines changed

‎packages/react-devtools-shared/src/__tests__/store-test.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2148,8 +2148,8 @@ describe('Store', () => {
21482148
act(() => render(<React.Fragment />));
21492149
});
21502150
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);
21532153
});
21542154

21552155
// Regression test for https://github.com/facebook/react/issues/23202

‎packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -420,8 +420,8 @@ describe('Store component filters', () => {
420420
});
421421

422422
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);
425425

426426
await actAsync(async () => (store.componentFilters = []));
427427
expect(store).toMatchInlineSnapshot(`
@@ -460,8 +460,8 @@ describe('Store component filters', () => {
460460
]),
461461
);
462462
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);
465465

466466
await actAsync(async () => (store.componentFilters = []));
467467
expect(store).toMatchInlineSnapshot(`
@@ -510,8 +510,8 @@ describe('Store component filters', () => {
510510
});
511511

512512
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);
515515

516516
await actAsync(async () => (store.componentFilters = []));
517517
expect(store).toMatchInlineSnapshot(`
@@ -550,8 +550,8 @@ describe('Store component filters', () => {
550550
]),
551551
);
552552
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);
555555

556556
await actAsync(async () => (store.componentFilters = []));
557557
expect(store).toMatchInlineSnapshot(`

‎packages/react-devtools-shared/src/devtools/store.js

Lines changed: 53 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -114,8 +114,8 @@ export default class Store extends EventEmitter<{
114114
_bridge: FrontendBridge;
115115

116116
// Computed whenever _errorsAndWarnings Map changes.
117-
_cachedErrorCount: number = 0;
118-
_cachedWarningCount: number = 0;
117+
_cachedComponentWithErrorCount: number = 0;
118+
_cachedComponentWithWarningCount: number = 0;
119119
_cachedErrorAndWarningTuples: ErrorAndWarningTuples | null = null;
120120

121121
// Should new nodes be collapsed by default when added to the tree?
@@ -196,6 +196,7 @@ export default class Store extends EventEmitter<{
196196

197197
_shouldCheckBridgeProtocolCompatibility: boolean = false;
198198
_hookSettings: $ReadOnly<DevToolsHookSettings> | null = null;
199+
_shouldShowWarningsAndErrors: boolean = false;
199200

200201
constructor(bridge: FrontendBridge, config?: Config) {
201202
super();
@@ -383,8 +384,24 @@ export default class Store extends EventEmitter<{
383384
return this._bridgeProtocol;
384385
}
385386

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;
388405
}
389406

390407
get hasOwnerMetadata(): boolean {
@@ -480,10 +497,6 @@ export default class Store extends EventEmitter<{
480497
return this._unsupportedRendererVersionDetected;
481498
}
482499

483-
get warningCount(): number {
484-
return this._cachedWarningCount;
485-
}
486-
487500
containsElement(id: number): boolean {
488501
return this._idToElement.has(id);
489502
}
@@ -581,7 +594,11 @@ export default class Store extends EventEmitter<{
581594
}
582595

583596
// Returns a tuple of [id, index]
584-
getElementsWithErrorsAndWarnings(): Array<{id: number, index: number}> {
597+
getElementsWithErrorsAndWarnings(): ErrorAndWarningTuples {
598+
if (!this._shouldShowWarningsAndErrors) {
599+
return [];
600+
}
601+
585602
if (this._cachedErrorAndWarningTuples !== null) {
586603
return this._cachedErrorAndWarningTuples;
587604
}
@@ -615,6 +632,10 @@ export default class Store extends EventEmitter<{
615632
errorCount: number,
616633
warningCount: number,
617634
} {
635+
if (!this._shouldShowWarningsAndErrors) {
636+
return {errorCount: 0, warningCount: 0};
637+
}
638+
618639
return this._errorsAndWarnings.get(id) || {errorCount: 0, warningCount: 0};
619640
}
620641

@@ -1325,16 +1346,21 @@ export default class Store extends EventEmitter<{
13251346
this._cachedErrorAndWarningTuples = null;
13261347

13271348
if (haveErrorsOrWarningsChanged) {
1328-
let errorCount = 0;
1329-
let warningCount = 0;
1349+
let componentWithErrorCount = 0;
1350+
let componentWithWarningCount = 0;
13301351

13311352
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+
}
13341360
});
13351361

1336-
this._cachedErrorCount = errorCount;
1337-
this._cachedWarningCount = warningCount;
1362+
this._cachedComponentWithErrorCount = componentWithErrorCount;
1363+
this._cachedComponentWithWarningCount = componentWithWarningCount;
13381364
}
13391365

13401366
if (haveRootsChanged) {
@@ -1528,9 +1554,21 @@ export default class Store extends EventEmitter<{
15281554
onHookSettings: (settings: $ReadOnly<DevToolsHookSettings>) => void =
15291555
settings => {
15301556
this._hookSettings = settings;
1557+
1558+
this.setShouldShowWarningsAndErrors(settings.showInlineWarningsAndErrors);
15311559
this.emit('hookSettings', settings);
15321560
};
15331561

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+
15341572
// The Store should never throw an Error without also emitting an event.
15351573
// Otherwise Store errors will be invisible to users,
15361574
// but the downstream errors they cause will be reported as bugs.

‎packages/react-devtools-shared/src/devtools/views/Components/Element.js

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,6 @@ import {Fragment, useContext, useMemo, useState} from 'react';
1212
import Store from 'react-devtools-shared/src/devtools/store';
1313
import ButtonIcon from '../ButtonIcon';
1414
import {TreeDispatcherContext, TreeStateContext} from './TreeContext';
15-
import {SettingsContext} from '../Settings/SettingsContext';
1615
import {StoreContext} from '../context';
1716
import {useSubscription} from '../hooks';
1817
import {logEvent} from 'react-devtools-shared/src/Logger';
@@ -37,7 +36,6 @@ export default function Element({data, index, style}: Props): React.Node {
3736
const {ownerFlatTree, ownerID, selectedElementID} =
3837
useContext(TreeStateContext);
3938
const dispatch = useContext(TreeDispatcherContext);
40-
const {showInlineWarningsAndErrors} = React.useContext(SettingsContext);
4139

4240
const element =
4341
ownerFlatTree !== null
@@ -181,7 +179,7 @@ export default function Element({data, index, style}: Props): React.Node {
181179
className={styles.BadgesBlock}
182180
/>
183181

184-
{showInlineWarningsAndErrors && errorCount > 0 && (
182+
{errorCount > 0 && (
185183
<Icon
186184
type="error"
187185
className={
@@ -191,7 +189,7 @@ export default function Element({data, index, style}: Props): React.Node {
191189
}
192190
/>
193191
)}
194-
{showInlineWarningsAndErrors && warningCount > 0 && (
192+
{warningCount > 0 && (
195193
<Icon
196194
type="warning"
197195
className={

‎packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorsAndWarningsTree.js

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@
99

1010
import * as React from 'react';
1111
import {
12-
useContext,
1312
unstable_useCacheRefresh as useCacheRefresh,
1413
useTransition,
1514
} from 'react';
@@ -18,7 +17,6 @@ import ButtonIcon from '../ButtonIcon';
1817
import Store from '../../store';
1918
import sharedStyles from './InspectedElementSharedStyles.css';
2019
import styles from './InspectedElementErrorsAndWarningsTree.css';
21-
import {SettingsContext} from '../Settings/SettingsContext';
2220
import {
2321
clearErrorsForElement as clearErrorsForElementAPI,
2422
clearWarningsForElement as clearWarningsForElementAPI,
@@ -74,12 +72,14 @@ export default function InspectedElementErrorsAndWarningsTree({
7472
}
7573
};
7674

77-
const {showInlineWarningsAndErrors} = useContext(SettingsContext);
78-
if (!showInlineWarningsAndErrors) {
75+
if (!store.displayingErrorsAndWarningsEnabled) {
7976
return null;
8077
}
8178

8279
const {errors, warnings} = inspectedElement;
80+
if (errors.length === 0 && warnings.length === 0) {
81+
return null;
82+
}
8383

8484
return (
8585
<React.Fragment>

‎packages/react-devtools-shared/src/devtools/views/Components/Tree.js

Lines changed: 35 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@ export default function Tree(props: Props): React.Node {
7373

7474
const [treeFocused, setTreeFocused] = useState<boolean>(false);
7575

76-
const {lineHeight, showInlineWarningsAndErrors} = useContext(SettingsContext);
76+
const {lineHeight} = useContext(SettingsContext);
7777

7878
// Make sure a newly selected element is visible in the list.
7979
// This is helpful for things like the owners list and search.
@@ -325,8 +325,8 @@ export default function Tree(props: Props): React.Node {
325325
const errorsOrWarningsSubscription = useMemo(
326326
() => ({
327327
getCurrentValue: () => ({
328-
errors: store.errorCount,
329-
warnings: store.warningCount,
328+
errors: store.componentWithErrorCount,
329+
warnings: store.componentWithWarningCount,
330330
}),
331331
subscribe: (callback: Function) => {
332332
store.addListener('mutated', callback);
@@ -370,40 +370,38 @@ export default function Tree(props: Props): React.Node {
370370
<Suspense fallback={<Loading />}>
371371
{ownerID !== null ? <OwnersStack /> : <ComponentSearchInput />}
372372
</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+
)}
407405
{!hideSettings && (
408406
<Fragment>
409407
<div className={styles.VRule} />

‎packages/react-devtools-shared/src/devtools/views/Settings/DebuggingSettings.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,10 @@ export default function DebuggingSettings({
3737
const [showInlineWarningsAndErrors, setShowInlineWarningsAndErrors] =
3838
useState(usedHookSettings.showInlineWarningsAndErrors);
3939

40+
useEffect(() => {
41+
store.setShouldShowWarningsAndErrors(showInlineWarningsAndErrors);
42+
}, [showInlineWarningsAndErrors]);
43+
4044
useEffect(() => {
4145
store.updateHookSettings({
4246
appendComponentStack,

‎packages/react-devtools-shared/src/devtools/views/Settings/SettingsContext.js

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -43,21 +43,9 @@ type Context = {
4343
// Specified as a separate prop so it can trigger a re-render of FixedSizeList.
4444
lineHeight: number,
4545

46-
appendComponentStack: boolean,
47-
setAppendComponentStack: (value: boolean) => void,
48-
49-
breakOnConsoleErrors: boolean,
50-
setBreakOnConsoleErrors: (value: boolean) => void,
51-
5246
parseHookNames: boolean,
5347
setParseHookNames: (value: boolean) => void,
5448

55-
hideConsoleLogsInStrictMode: boolean,
56-
setHideConsoleLogsInStrictMode: (value: boolean) => void,
57-
58-
showInlineWarningsAndErrors: boolean,
59-
setShowInlineWarningsAndErrors: (value: boolean) => void,
60-
6149
theme: Theme,
6250
setTheme(value: Theme): void,
6351

@@ -176,7 +164,7 @@ function SettingsContextController({
176164
bridge.send('setTraceUpdatesEnabled', traceUpdatesEnabled);
177165
}, [bridge, traceUpdatesEnabled]);
178166

179-
const value = useMemo(
167+
const value: Context = useMemo(
180168
() => ({
181169
displayDensity,
182170
lineHeight:

0 commit comments

Comments
 (0)