diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index d5b9c3cbc9213..1a283fe120ef3 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -1075,40 +1075,110 @@ describe('Store', () => { `); }); - // This is not great, but it seems safer than potentially flushing between commits. - // Our logic for determining how to handle e.g. suspended trees or error boundaries - // is built on the assumption that we're evaluating the results of a commit, not an in-progress render. - it('during passive get counted (but not until the next commit)', () => { - function Example() { - React.useEffect(() => { - console.error('test-only: passive error'); - console.warn('test-only: passive warning'); - }); - return null; + describe('during passive effects', () => { + function flushPendingBridgeOperations() { + jest.runOnlyPendingTimers(); } - const container = document.createElement('div'); - - withErrorsOrWarningsIgnored(['test-only:'], () => { - act(() => ReactDOM.render(, container)); - }); - expect(store).toMatchInlineSnapshot(` - [root] - - `); + // Gross abstraction around pending passive warning/error delay. + function flushPendingPassiveErrorAndWarningCounts() { + jest.advanceTimersByTime(1000); + } - withErrorsOrWarningsIgnored(['test-only:'], () => { - act(() => ReactDOM.render(, container)); + it('are counted (after a delay)', () => { + function Example() { + React.useEffect(() => { + console.error('test-only: passive error'); + console.warn('test-only: passive warning'); + }); + return null; + } + const container = document.createElement('div'); + + withErrorsOrWarningsIgnored(['test-only:'], () => { + act(() => { + ReactDOM.render(, container); + }, 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] + ✕⚠ + `); + + act(() => ReactDOM.unmountComponentAtNode(container)); + expect(store).toMatchInlineSnapshot(``); }); - expect(store).toMatchInlineSnapshot(` - ✕ 1, ⚠ 1 - [root] - ✕⚠ - `); + it('are flushed early when there is a new commit', () => { + function Example() { + React.useEffect(() => { + console.error('test-only: passive error'); + console.warn('test-only: passive warning'); + }); + return null; + } + + function Noop() { + return null; + } + + const container = document.createElement('div'); + + withErrorsOrWarningsIgnored(['test-only:'], () => { + act(() => { + ReactDOM.render( + <> + + , + container, + ); + }, false); + flushPendingBridgeOperations(); + expect(store).toMatchInlineSnapshot(` + [root] + + `); + + // Before warnings and errors have flushed, flush another commit. + act(() => { + ReactDOM.render( + <> + + + , + container, + ); + }, false); + flushPendingBridgeOperations(); + expect(store).toMatchInlineSnapshot(` + ✕ 1, ⚠ 1 + [root] + ✕⚠ + + `); + }); - act(() => ReactDOM.unmountComponentAtNode(container)); - expect(store).toMatchInlineSnapshot(``); + // After a delay, passive effects should be committed as well + act(flushPendingPassiveErrorAndWarningCounts, false); + expect(store).toMatchInlineSnapshot(` + ✕ 2, ⚠ 2 + [root] + ✕⚠ + + `); + + act(() => ReactDOM.unmountComponentAtNode(container)); + expect(store).toMatchInlineSnapshot(``); + }); }); it('from react get counted', () => { diff --git a/packages/react-devtools-shared/src/__tests__/utils.js b/packages/react-devtools-shared/src/__tests__/utils.js index 2fb0664a24855..61a6acc604a70 100644 --- a/packages/react-devtools-shared/src/__tests__/utils.js +++ b/packages/react-devtools-shared/src/__tests__/utils.js @@ -14,7 +14,10 @@ import type Store from 'react-devtools-shared/src/devtools/store'; import type {ProfilingDataFrontend} from 'react-devtools-shared/src/devtools/views/Profiler/types'; import type {ElementType} from 'react-devtools-shared/src/types'; -export function act(callback: Function): void { +export function act( + callback: Function, + recursivelyFlush: boolean = true, +): void { const {act: actTestRenderer} = require('react-test-renderer'); const {act: actDOM} = require('react-dom/test-utils'); @@ -24,13 +27,15 @@ export function act(callback: Function): void { }); }); - // Flush Bridge operations - while (jest.getTimerCount() > 0) { - actDOM(() => { - actTestRenderer(() => { - jest.runAllTimers(); + if (recursivelyFlush) { + // Flush Bridge operations + while (jest.getTimerCount() > 0) { + actDOM(() => { + actTestRenderer(() => { + jest.runAllTimers(); + }); }); - }); + } } } diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index c5f6932234a81..b23b59f73d84a 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -626,6 +626,14 @@ export function attach( // If this Fiber is currently being inspected, mark it as needing an udpate as well. updateMostRecentlyInspectedElementIfNecessary(fiberID); + + // Passive effects may trigger errors or warnings too; + // In this case, we should wait until the rest of the passive effects have run, + // but we shouldn't wait until the next commit because that might be a long time. + // This would also cause "tearing" between an inspected Component and the tree view. + // Then again we don't want to flush too soon because this could be an error during async rendering. + // Use a debounce technique to ensure that we'll eventually flush. + flushPendingErrorsAndWarningsAfterDelay(); } // Patching the console enables DevTools to do a few useful things: @@ -1198,10 +1206,12 @@ export function attach( } } - const pendingOperations: Array = []; + type OperationsArray = Array; + + const pendingOperations: OperationsArray = []; const pendingRealUnmountedIDs: Array = []; const pendingSimulatedUnmountedIDs: Array = []; - let pendingOperationsQueue: Array> | null = []; + let pendingOperationsQueue: Array | null = []; const pendingStringTable: Map = new Map(); let pendingStringTableLength: number = 0; let pendingUnmountedRootID: number | null = null; @@ -1218,6 +1228,61 @@ export function attach( pendingOperations.push(op); } + function flushOrQueueOperations(operations: OperationsArray): void { + if (pendingOperationsQueue !== null) { + pendingOperationsQueue.push(operations); + } else { + hook.emit('operations', operations); + } + } + + let flushPendingErrorsAndWarningsAfterDelayTimeoutID = null; + + function clearPendingErrorsAndWarningsAfterDelay() { + if (flushPendingErrorsAndWarningsAfterDelayTimeoutID !== null) { + clearTimeout(flushPendingErrorsAndWarningsAfterDelayTimeoutID); + flushPendingErrorsAndWarningsAfterDelayTimeoutID = null; + } + } + + function flushPendingErrorsAndWarningsAfterDelay() { + clearPendingErrorsAndWarningsAfterDelay(); + + flushPendingErrorsAndWarningsAfterDelayTimeoutID = setTimeout(() => { + flushPendingErrorsAndWarningsAfterDelayTimeoutID = null; + + if (pendingOperations.length > 0) { + // On the off chance that something else has pushed pending operations, + // we should bail on warnings; it's probably not safe to push midway. + return; + } + + recordPendingErrorsAndWarnings(); + + if (pendingOperations.length === 0) { + // No warnings or errors to flush; we can bail out early here too. + return; + } + + // We can create a smaller operations array than flushPendingEvents() + // because we only need to flush warning and error counts. + // Only a few pieces of fixed information are required up front. + const operations: OperationsArray = new Array( + 3 + pendingOperations.length, + ); + operations[0] = rendererID; + operations[1] = currentRootID; + operations[2] = 0; // String table size + for (let j = 0; j < pendingOperations.length; j++) { + operations[3 + j] = pendingOperations[j]; + } + + flushOrQueueOperations(operations); + + pendingOperations.length = 0; + }, 1000); + } + function reevaluateErrorsAndWarnings() { fibersWithChangedErrorOrWarningCounts.clear(); fiberToErrorsMap.forEach((countMap, fiberID) => { @@ -1230,6 +1295,8 @@ export function attach( } function recordPendingErrorsAndWarnings() { + clearPendingErrorsAndWarningsAfterDelay(); + fibersWithChangedErrorOrWarningCounts.forEach(fiberID => { const fiber = idToFiberMap.get(fiberID); if (fiber != null) { @@ -1319,7 +1386,7 @@ export function attach( // Which in turn enables fiber props, states, and hooks to be inspected. let i = 0; operations[i++] = rendererID; - operations[i++] = currentRootID; // Use this ID in case the root was unmounted! + operations[i++] = currentRootID; // Now fill in the string table. // [stringTableLength, str1Length, ...str1, str2Length, ...str2, ...] @@ -1366,18 +1433,9 @@ export function attach( i += pendingOperations.length; // Let the frontend know about tree operations. - // The first value in this array will identify which root it corresponds to, - // so we do no longer need to dispatch a separate root-committed event. - if (pendingOperationsQueue !== null) { - // Until the frontend has been connected, store the tree operations. - // This will let us avoid walking the tree later when the frontend connects, - // and it enables the Profiler's reload-and-profile functionality to work as well. - pendingOperationsQueue.push(operations); - } else { - // If we've already connected to the frontend, just pass the operations through. - hook.emit('operations', operations); - } + flushOrQueueOperations(operations); + // Reset all of the pending state now that we've told the frontend about it. pendingOperations.length = 0; pendingRealUnmountedIDs.length = 0; pendingSimulatedUnmountedIDs.length = 0;