From a15bbe14751287cb7ac124ff88f694d0883f3ac6 Mon Sep 17 00:00:00 2001 From: Ruslan Lesiutin Date: Tue, 24 Sep 2024 19:51:21 +0100 Subject: [PATCH] refactor: data source for errors and warnings tracking is now in Store (#31010) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../src/__tests__/store-test.js | 4 +- .../__tests__/storeComponentFilters-test.js | 16 ++--- .../src/devtools/store.js | 68 ++++++++++++++---- .../src/devtools/views/Components/Element.js | 6 +- .../InspectedElementErrorsAndWarningsTree.js | 8 +-- .../src/devtools/views/Components/Tree.js | 72 +++++++++---------- .../views/Settings/DebuggingSettings.js | 4 ++ .../views/Settings/SettingsContext.js | 14 +--- 8 files changed, 109 insertions(+), 83 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index 92e7fc6586111..93c7048b2bc5b 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -2148,8 +2148,8 @@ describe('Store', () => { act(() => render()); }); expect(store).toMatchInlineSnapshot(`[root]`); - expect(store.errorCount).toBe(0); - expect(store.warningCount).toBe(0); + expect(store.componentWithErrorCount).toBe(0); + expect(store.componentWithWarningCount).toBe(0); }); // Regression test for https://github.com/facebook/react/issues/23202 diff --git a/packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js b/packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js index 432fb1771b6ff..26cd51383297f 100644 --- a/packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js +++ b/packages/react-devtools-shared/src/__tests__/storeComponentFilters-test.js @@ -420,8 +420,8 @@ describe('Store component filters', () => { }); expect(store).toMatchInlineSnapshot(``); - expect(store.errorCount).toBe(0); - expect(store.warningCount).toBe(0); + expect(store.componentWithErrorCount).toBe(0); + expect(store.componentWithWarningCount).toBe(0); await actAsync(async () => (store.componentFilters = [])); expect(store).toMatchInlineSnapshot(` @@ -460,8 +460,8 @@ describe('Store component filters', () => { ]), ); expect(store).toMatchInlineSnapshot(`[root]`); - expect(store.errorCount).toBe(0); - expect(store.warningCount).toBe(0); + expect(store.componentWithErrorCount).toBe(0); + expect(store.componentWithWarningCount).toBe(0); await actAsync(async () => (store.componentFilters = [])); expect(store).toMatchInlineSnapshot(` @@ -510,8 +510,8 @@ describe('Store component filters', () => { }); expect(store).toMatchInlineSnapshot(``); - expect(store.errorCount).toBe(0); - expect(store.warningCount).toBe(0); + expect(store.componentWithErrorCount).toBe(0); + expect(store.componentWithWarningCount).toBe(0); await actAsync(async () => (store.componentFilters = [])); expect(store).toMatchInlineSnapshot(` @@ -550,8 +550,8 @@ describe('Store component filters', () => { ]), ); expect(store).toMatchInlineSnapshot(`[root]`); - expect(store.errorCount).toBe(0); - expect(store.warningCount).toBe(0); + expect(store.componentWithErrorCount).toBe(0); + expect(store.componentWithWarningCount).toBe(0); await actAsync(async () => (store.componentFilters = [])); expect(store).toMatchInlineSnapshot(` diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index b54907338b372..b1544126d8d6e 100644 --- a/packages/react-devtools-shared/src/devtools/store.js +++ b/packages/react-devtools-shared/src/devtools/store.js @@ -114,8 +114,8 @@ export default class Store extends EventEmitter<{ _bridge: FrontendBridge; // Computed whenever _errorsAndWarnings Map changes. - _cachedErrorCount: number = 0; - _cachedWarningCount: number = 0; + _cachedComponentWithErrorCount: number = 0; + _cachedComponentWithWarningCount: number = 0; _cachedErrorAndWarningTuples: ErrorAndWarningTuples | null = null; // Should new nodes be collapsed by default when added to the tree? @@ -196,6 +196,7 @@ export default class Store extends EventEmitter<{ _shouldCheckBridgeProtocolCompatibility: boolean = false; _hookSettings: $ReadOnly | null = null; + _shouldShowWarningsAndErrors: boolean = false; constructor(bridge: FrontendBridge, config?: Config) { super(); @@ -383,8 +384,24 @@ export default class Store extends EventEmitter<{ return this._bridgeProtocol; } - get errorCount(): number { - return this._cachedErrorCount; + get componentWithErrorCount(): number { + if (!this._shouldShowWarningsAndErrors) { + return 0; + } + + return this._cachedComponentWithErrorCount; + } + + get componentWithWarningCount(): number { + if (!this._shouldShowWarningsAndErrors) { + return 0; + } + + return this._cachedComponentWithWarningCount; + } + + get displayingErrorsAndWarningsEnabled(): boolean { + return this._shouldShowWarningsAndErrors; } get hasOwnerMetadata(): boolean { @@ -480,10 +497,6 @@ export default class Store extends EventEmitter<{ return this._unsupportedRendererVersionDetected; } - get warningCount(): number { - return this._cachedWarningCount; - } - containsElement(id: number): boolean { return this._idToElement.has(id); } @@ -581,7 +594,11 @@ export default class Store extends EventEmitter<{ } // Returns a tuple of [id, index] - getElementsWithErrorsAndWarnings(): Array<{id: number, index: number}> { + getElementsWithErrorsAndWarnings(): ErrorAndWarningTuples { + if (!this._shouldShowWarningsAndErrors) { + return []; + } + if (this._cachedErrorAndWarningTuples !== null) { return this._cachedErrorAndWarningTuples; } @@ -615,6 +632,10 @@ export default class Store extends EventEmitter<{ errorCount: number, warningCount: number, } { + if (!this._shouldShowWarningsAndErrors) { + return {errorCount: 0, warningCount: 0}; + } + return this._errorsAndWarnings.get(id) || {errorCount: 0, warningCount: 0}; } @@ -1325,16 +1346,21 @@ export default class Store extends EventEmitter<{ this._cachedErrorAndWarningTuples = null; if (haveErrorsOrWarningsChanged) { - let errorCount = 0; - let warningCount = 0; + let componentWithErrorCount = 0; + let componentWithWarningCount = 0; this._errorsAndWarnings.forEach(entry => { - errorCount += entry.errorCount; - warningCount += entry.warningCount; + if (entry.errorCount > 0) { + componentWithErrorCount++; + } + + if (entry.warningCount > 0) { + componentWithWarningCount++; + } }); - this._cachedErrorCount = errorCount; - this._cachedWarningCount = warningCount; + this._cachedComponentWithErrorCount = componentWithErrorCount; + this._cachedComponentWithWarningCount = componentWithWarningCount; } if (haveRootsChanged) { @@ -1528,9 +1554,21 @@ export default class Store extends EventEmitter<{ onHookSettings: (settings: $ReadOnly) => void = settings => { this._hookSettings = settings; + + this.setShouldShowWarningsAndErrors(settings.showInlineWarningsAndErrors); this.emit('hookSettings', settings); }; + setShouldShowWarningsAndErrors(status: boolean): void { + const previousStatus = this._shouldShowWarningsAndErrors; + this._shouldShowWarningsAndErrors = status; + + if (previousStatus !== status) { + // Propagate to subscribers, although tree state has not changed + this.emit('mutated', [[], new Map()]); + } + } + // 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. diff --git a/packages/react-devtools-shared/src/devtools/views/Components/Element.js b/packages/react-devtools-shared/src/devtools/views/Components/Element.js index f5c30d2d2b0d6..48bfbe90906ff 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/Element.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/Element.js @@ -12,7 +12,6 @@ import {Fragment, useContext, useMemo, useState} from 'react'; import Store from 'react-devtools-shared/src/devtools/store'; import ButtonIcon from '../ButtonIcon'; import {TreeDispatcherContext, TreeStateContext} from './TreeContext'; -import {SettingsContext} from '../Settings/SettingsContext'; import {StoreContext} from '../context'; import {useSubscription} from '../hooks'; import {logEvent} from 'react-devtools-shared/src/Logger'; @@ -37,7 +36,6 @@ export default function Element({data, index, style}: Props): React.Node { const {ownerFlatTree, ownerID, selectedElementID} = useContext(TreeStateContext); const dispatch = useContext(TreeDispatcherContext); - const {showInlineWarningsAndErrors} = React.useContext(SettingsContext); const element = ownerFlatTree !== null @@ -181,7 +179,7 @@ export default function Element({data, index, style}: Props): React.Node { className={styles.BadgesBlock} /> - {showInlineWarningsAndErrors && errorCount > 0 && ( + {errorCount > 0 && ( )} - {showInlineWarningsAndErrors && warningCount > 0 && ( + {warningCount > 0 && ( diff --git a/packages/react-devtools-shared/src/devtools/views/Components/Tree.js b/packages/react-devtools-shared/src/devtools/views/Components/Tree.js index 136baa1205de2..db1bf9a98c135 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/Tree.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/Tree.js @@ -73,7 +73,7 @@ export default function Tree(props: Props): React.Node { const [treeFocused, setTreeFocused] = useState(false); - const {lineHeight, showInlineWarningsAndErrors} = useContext(SettingsContext); + const {lineHeight} = useContext(SettingsContext); // Make sure a newly selected element is visible in the list. // This is helpful for things like the owners list and search. @@ -325,8 +325,8 @@ export default function Tree(props: Props): React.Node { const errorsOrWarningsSubscription = useMemo( () => ({ getCurrentValue: () => ({ - errors: store.errorCount, - warnings: store.warningCount, + errors: store.componentWithErrorCount, + warnings: store.componentWithWarningCount, }), subscribe: (callback: Function) => { store.addListener('mutated', callback); @@ -370,40 +370,38 @@ export default function Tree(props: Props): React.Node { }> {ownerID !== null ? : } - {showInlineWarningsAndErrors && - ownerID === null && - (errors > 0 || warnings > 0) && ( - -
- {errors > 0 && ( -
- - {errors} -
- )} - {warnings > 0 && ( -
- - {warnings} -
- )} - - - - - )} + {ownerID === null && (errors > 0 || warnings > 0) && ( + +
+ {errors > 0 && ( +
+ + {errors} +
+ )} + {warnings > 0 && ( +
+ + {warnings} +
+ )} + + + + + )} {!hideSettings && (
diff --git a/packages/react-devtools-shared/src/devtools/views/Settings/DebuggingSettings.js b/packages/react-devtools-shared/src/devtools/views/Settings/DebuggingSettings.js index 8bde14e62e606..c48cdb58e3e4c 100644 --- a/packages/react-devtools-shared/src/devtools/views/Settings/DebuggingSettings.js +++ b/packages/react-devtools-shared/src/devtools/views/Settings/DebuggingSettings.js @@ -37,6 +37,10 @@ export default function DebuggingSettings({ const [showInlineWarningsAndErrors, setShowInlineWarningsAndErrors] = useState(usedHookSettings.showInlineWarningsAndErrors); + useEffect(() => { + store.setShouldShowWarningsAndErrors(showInlineWarningsAndErrors); + }, [showInlineWarningsAndErrors]); + useEffect(() => { store.updateHookSettings({ appendComponentStack, diff --git a/packages/react-devtools-shared/src/devtools/views/Settings/SettingsContext.js b/packages/react-devtools-shared/src/devtools/views/Settings/SettingsContext.js index 17514d94648ac..196ea806f6aac 100644 --- a/packages/react-devtools-shared/src/devtools/views/Settings/SettingsContext.js +++ b/packages/react-devtools-shared/src/devtools/views/Settings/SettingsContext.js @@ -43,21 +43,9 @@ type Context = { // Specified as a separate prop so it can trigger a re-render of FixedSizeList. lineHeight: number, - appendComponentStack: boolean, - setAppendComponentStack: (value: boolean) => void, - - breakOnConsoleErrors: boolean, - setBreakOnConsoleErrors: (value: boolean) => void, - parseHookNames: boolean, setParseHookNames: (value: boolean) => void, - hideConsoleLogsInStrictMode: boolean, - setHideConsoleLogsInStrictMode: (value: boolean) => void, - - showInlineWarningsAndErrors: boolean, - setShowInlineWarningsAndErrors: (value: boolean) => void, - theme: Theme, setTheme(value: Theme): void, @@ -176,7 +164,7 @@ function SettingsContextController({ bridge.send('setTraceUpdatesEnabled', traceUpdatesEnabled); }, [bridge, traceUpdatesEnabled]); - const value = useMemo( + const value: Context = useMemo( () => ({ displayDensity, lineHeight: