Skip to content

Commit

Permalink
refactor: data source for errors and warnings tracking is now in Store (
Browse files Browse the repository at this point in the history
#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
  • Loading branch information
hoxyq committed Sep 24, 2024
1 parent fc4a33e commit a15bbe1
Show file tree
Hide file tree
Showing 8 changed files with 109 additions and 83 deletions.
4 changes: 2 additions & 2 deletions packages/react-devtools-shared/src/__tests__/store-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -2148,8 +2148,8 @@ describe('Store', () => {
act(() => render(<React.Fragment />));
});
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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(`
Expand Down Expand Up @@ -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(`
Expand Down Expand Up @@ -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(`
Expand Down Expand Up @@ -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(`
Expand Down
68 changes: 53 additions & 15 deletions packages/react-devtools-shared/src/devtools/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down Expand Up @@ -196,6 +196,7 @@ export default class Store extends EventEmitter<{

_shouldCheckBridgeProtocolCompatibility: boolean = false;
_hookSettings: $ReadOnly<DevToolsHookSettings> | null = null;
_shouldShowWarningsAndErrors: boolean = false;

constructor(bridge: FrontendBridge, config?: Config) {
super();
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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};
}

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -1528,9 +1554,21 @@ export default class Store extends EventEmitter<{
onHookSettings: (settings: $ReadOnly<DevToolsHookSettings>) => 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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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
Expand Down Expand Up @@ -181,7 +179,7 @@ export default function Element({data, index, style}: Props): React.Node {
className={styles.BadgesBlock}
/>

{showInlineWarningsAndErrors && errorCount > 0 && (
{errorCount > 0 && (
<Icon
type="error"
className={
Expand All @@ -191,7 +189,7 @@ export default function Element({data, index, style}: Props): React.Node {
}
/>
)}
{showInlineWarningsAndErrors && warningCount > 0 && (
{warningCount > 0 && (
<Icon
type="warning"
className={
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

import * as React from 'react';
import {
useContext,
unstable_useCacheRefresh as useCacheRefresh,
useTransition,
} from 'react';
Expand All @@ -18,7 +17,6 @@ import ButtonIcon from '../ButtonIcon';
import Store from '../../store';
import sharedStyles from './InspectedElementSharedStyles.css';
import styles from './InspectedElementErrorsAndWarningsTree.css';
import {SettingsContext} from '../Settings/SettingsContext';
import {
clearErrorsForElement as clearErrorsForElementAPI,
clearWarningsForElement as clearWarningsForElementAPI,
Expand Down Expand Up @@ -74,12 +72,14 @@ export default function InspectedElementErrorsAndWarningsTree({
}
};

const {showInlineWarningsAndErrors} = useContext(SettingsContext);
if (!showInlineWarningsAndErrors) {
if (!store.displayingErrorsAndWarningsEnabled) {
return null;
}

const {errors, warnings} = inspectedElement;
if (errors.length === 0 && warnings.length === 0) {
return null;
}

return (
<React.Fragment>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ export default function Tree(props: Props): React.Node {

const [treeFocused, setTreeFocused] = useState<boolean>(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.
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -370,40 +370,38 @@ export default function Tree(props: Props): React.Node {
<Suspense fallback={<Loading />}>
{ownerID !== null ? <OwnersStack /> : <ComponentSearchInput />}
</Suspense>
{showInlineWarningsAndErrors &&
ownerID === null &&
(errors > 0 || warnings > 0) && (
<React.Fragment>
<div className={styles.VRule} />
{errors > 0 && (
<div className={styles.IconAndCount}>
<Icon className={styles.ErrorIcon} type="error" />
{errors}
</div>
)}
{warnings > 0 && (
<div className={styles.IconAndCount}>
<Icon className={styles.WarningIcon} type="warning" />
{warnings}
</div>
)}
<Button
onClick={handlePreviousErrorOrWarningClick}
title="Scroll to previous error or warning">
<ButtonIcon type="up" />
</Button>
<Button
onClick={handleNextErrorOrWarningClick}
title="Scroll to next error or warning">
<ButtonIcon type="down" />
</Button>
<Button
onClick={clearErrorsAndWarnings}
title="Clear all errors and warnings">
<ButtonIcon type="clear" />
</Button>
</React.Fragment>
)}
{ownerID === null && (errors > 0 || warnings > 0) && (
<React.Fragment>
<div className={styles.VRule} />
{errors > 0 && (
<div className={styles.IconAndCount}>
<Icon className={styles.ErrorIcon} type="error" />
{errors}
</div>
)}
{warnings > 0 && (
<div className={styles.IconAndCount}>
<Icon className={styles.WarningIcon} type="warning" />
{warnings}
</div>
)}
<Button
onClick={handlePreviousErrorOrWarningClick}
title="Scroll to previous error or warning">
<ButtonIcon type="up" />
</Button>
<Button
onClick={handleNextErrorOrWarningClick}
title="Scroll to next error or warning">
<ButtonIcon type="down" />
</Button>
<Button
onClick={clearErrorsAndWarnings}
title="Clear all errors and warnings">
<ButtonIcon type="clear" />
</Button>
</React.Fragment>
)}
{!hideSettings && (
<Fragment>
<div className={styles.VRule} />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ export default function DebuggingSettings({
const [showInlineWarningsAndErrors, setShowInlineWarningsAndErrors] =
useState(usedHookSettings.showInlineWarningsAndErrors);

useEffect(() => {
store.setShouldShowWarningsAndErrors(showInlineWarningsAndErrors);
}, [showInlineWarningsAndErrors]);

useEffect(() => {
store.updateHookSettings({
appendComponentStack,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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,

Expand Down Expand Up @@ -176,7 +164,7 @@ function SettingsContextController({
bridge.send('setTraceUpdatesEnabled', traceUpdatesEnabled);
}, [bridge, traceUpdatesEnabled]);

const value = useMemo(
const value: Context = useMemo(
() => ({
displayDensity,
lineHeight:
Expand Down

0 comments on commit a15bbe1

Please sign in to comment.