Skip to content

Commit

Permalink
Brute force error/warning count to update if it happens in passive ef…
Browse files Browse the repository at this point in the history
…fects

If a log is added in a passive effect it is too late for us to have picked
it up in the tree traversal so it won't show up until that component is
rerendered again.

To avoid needing a map to look up the instance id, we do a brute force
search over the whole tree to find any changes if this happens.
  • Loading branch information
sebmarkbage committed Sep 6, 2024
1 parent 006e1d7 commit 56de3e4
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 25 deletions.
28 changes: 4 additions & 24 deletions packages/react-devtools-shared/src/__tests__/store-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1817,13 +1817,8 @@ describe('Store', () => {
jest.runOnlyPendingTimers();
}

// Gross abstraction around pending passive warning/error delay.
function flushPendingPassiveErrorAndWarningCounts() {
jest.advanceTimersByTime(1000);
}

// @reactVersion >= 18.0
it('are counted (after a delay)', () => {
it('are counted (after no delay)', () => {
function Example() {
React.useEffect(() => {
console.error('test-only: passive error');
Expand All @@ -1838,13 +1833,6 @@ describe('Store', () => {
}, false);
});
flushPendingBridgeOperations();
expect(store).toMatchInlineSnapshot(`
[root]
<Example>
`);

// After a delay, passive effects should be committed as well
act(flushPendingPassiveErrorAndWarningCounts, false);
expect(store).toMatchInlineSnapshot(`
✕ 1, ⚠ 1
[root]
Expand Down Expand Up @@ -1879,8 +1867,9 @@ describe('Store', () => {
}, false);
flushPendingBridgeOperations();
expect(store).toMatchInlineSnapshot(`
✕ 1, ⚠ 1
[root]
<Example>
<Example> ✕⚠
`);

// Before warnings and errors have flushed, flush another commit.
Expand All @@ -1894,22 +1883,13 @@ describe('Store', () => {
}, false);
flushPendingBridgeOperations();
expect(store).toMatchInlineSnapshot(`
1, ⚠ 1
2, ⚠ 2
[root]
<Example> ✕⚠
<Noop>
`);
});

// After a delay, passive effects should be committed as well
act(flushPendingPassiveErrorAndWarningCounts, false);
expect(store).toMatchInlineSnapshot(`
✕ 2, ⚠ 2
[root]
<Example> ✕⚠
<Noop>
`);

act(() => unmount());
expect(store).toMatchInlineSnapshot(``);
});
Expand Down
45 changes: 44 additions & 1 deletion packages/react-devtools-shared/src/backend/fiber/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -970,6 +970,32 @@ export function attach(
// lets the Fiber get reparented/remounted and still observe the previous errors/warnings.
// Unless we explicitly clear the logs from a Fiber.
const fiberToComponentLogsMap: WeakMap<Fiber, ComponentLogs> = new WeakMap();
// Tracks whether we've performed a commit since the last log. This is used to know
// whether we received any new logs between the commit and post commit phases. I.e.
// if any passive effects called console.warn / console.error.
let needsToFlushComponentLogs = false;

function bruteForceFlushErrorsAndWarnings() {
// Refresh error/warning count for all mounted unfiltered Fibers.
let hasChanges = false;
// eslint-disable-next-line no-for-of-loops/no-for-of-loops
for (const devtoolsInstance of idToDevToolsInstanceMap.values()) {
if (devtoolsInstance.kind === FIBER_INSTANCE) {
const fiber = devtoolsInstance.data;
const componentLogsEntry = fiberToComponentLogsMap.get(fiber);
const changed = recordConsoleLogs(devtoolsInstance, componentLogsEntry);
if (changed) {
hasChanges = true;
updateMostRecentlyInspectedElementIfNecessary(devtoolsInstance.id);
}
} else {
// Virtual Instances cannot log in passive effects and so never appear here.
}
}
if (hasChanges) {
flushPendingEvents();
}
}

function clearErrorsAndWarnings() {
// Note, this only clears logs for Fibers that have instances. If they're filtered
Expand Down Expand Up @@ -1101,10 +1127,12 @@ export function attach(
}

// The changes will be flushed later when we commit.
// TODO: If the log happened in a passive effect, then this happens after we've

// If the log happened in a passive effect, then this happens after we've
// already committed the new tree so the change won't show up until we rerender
// that component again. We need to visit a Component with passive effects in
// handlePostCommitFiberRoot again to ensure that we flush the changes after passive.
needsToFlushComponentLogs = true;
}

// Patching the console enables DevTools to do a few useful things:
Expand Down Expand Up @@ -1322,6 +1350,8 @@ export function attach(
});

flushPendingEvents();

needsToFlushComponentLogs = false;
}

function getEnvironmentNames(): Array<string> {
Expand Down Expand Up @@ -3464,6 +3494,8 @@ export function attach(
mountFiberRecursively(root.current, false);

flushPendingEvents(root);

needsToFlushComponentLogs = false;
currentRoot = (null: any);
});
}
Expand All @@ -3486,6 +3518,15 @@ export function attach(
passiveEffectDuration;
}
}

if (needsToFlushComponentLogs) {
// We received new logs after commit. I.e. in a passive effect. We need to
// traverse the tree to find the affected ones. If we just moved the whole
// tree traversal from handleCommitFiberRoot to handlePostCommitFiberRoot
// this wouldn't be needed. For now we just brute force check all instances.
// This is not that common of a case.
bruteForceFlushErrorsAndWarnings();
}
}

function handleCommitFiberRoot(
Expand Down Expand Up @@ -3595,6 +3636,8 @@ export function attach(
// We're done here.
flushPendingEvents(root);

needsToFlushComponentLogs = false;

if (traceUpdatesEnabled) {
hook.emit('traceUpdates', traceUpdatesForNodes);
}
Expand Down

0 comments on commit 56de3e4

Please sign in to comment.