From 56de3e45a1885c1168b7557651e4e45cbbe5106f Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Fri, 6 Sep 2024 18:01:18 -0400 Subject: [PATCH] Brute force error/warning count to update if it happens in passive effects 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. --- .../src/__tests__/store-test.js | 28 ++---------- .../src/backend/fiber/renderer.js | 45 ++++++++++++++++++- 2 files changed, 48 insertions(+), 25 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index cebc7e0bbcb82..92e7fc6586111 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -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'); @@ -1838,13 +1833,6 @@ describe('Store', () => { }, false); }); flushPendingBridgeOperations(); - expect(store).toMatchInlineSnapshot(` - [root] - - `); - - // After a delay, passive effects should be committed as well - act(flushPendingPassiveErrorAndWarningCounts, false); expect(store).toMatchInlineSnapshot(` ✕ 1, ⚠ 1 [root] @@ -1879,8 +1867,9 @@ describe('Store', () => { }, false); flushPendingBridgeOperations(); expect(store).toMatchInlineSnapshot(` + ✕ 1, ⚠ 1 [root] - + ✕⚠ `); // Before warnings and errors have flushed, flush another commit. @@ -1894,22 +1883,13 @@ describe('Store', () => { }, false); flushPendingBridgeOperations(); expect(store).toMatchInlineSnapshot(` - ✕ 1, ⚠ 1 + ✕ 2, ⚠ 2 [root] ✕⚠ `); }); - // After a delay, passive effects should be committed as well - act(flushPendingPassiveErrorAndWarningCounts, false); - expect(store).toMatchInlineSnapshot(` - ✕ 2, ⚠ 2 - [root] - ✕⚠ - - `); - act(() => unmount()); expect(store).toMatchInlineSnapshot(``); }); diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 82fbb526bad4d..189da54c8603b 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -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 = 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 @@ -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: @@ -1322,6 +1350,8 @@ export function attach( }); flushPendingEvents(); + + needsToFlushComponentLogs = false; } function getEnvironmentNames(): Array { @@ -3464,6 +3494,8 @@ export function attach( mountFiberRecursively(root.current, false); flushPendingEvents(root); + + needsToFlushComponentLogs = false; currentRoot = (null: any); }); } @@ -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( @@ -3595,6 +3636,8 @@ export function attach( // We're done here. flushPendingEvents(root); + needsToFlushComponentLogs = false; + if (traceUpdatesEnabled) { hook.emit('traceUpdates', traceUpdatesForNodes); }