diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index d1a04b74b5b9d..1a283fe120ef3 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -1075,43 +1075,110 @@ describe('Store', () => { `); }); - it('during passive get counted (after a delay)', () => { - 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); - // flush bridge operations - jest.runOnlyPendingTimers(); - }, false); - }); - expect(store).toMatchInlineSnapshot(` - [root] - - `); - - // flush count after delay - act(() => { + // Gross abstraction around pending passive warning/error delay. + function flushPendingPassiveErrorAndWarningCounts() { jest.advanceTimersByTime(1000); - }, false); + } - // After a delay, passive effects should be committed as well - expect(store).toMatchInlineSnapshot(` - ✕ 1, ⚠ 1 - [root] - ✕⚠ - `); + 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(``); + }); - act(() => ReactDOM.unmountComponentAtNode(container)); - expect(store).toMatchInlineSnapshot(``); + 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] + ✕⚠ + + `); + }); + + // 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/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 6a4e4d30549f5..d415391e234fa 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -631,7 +631,8 @@ export function attach( // 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 rendering might stil be going on. + // 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(); } @@ -1205,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; @@ -1225,17 +1228,30 @@ 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 flushPendingErrorsAndWarningsAfterDelay() { + + function clearPendingErrorsAndWarningsAfterDelay() { if (flushPendingErrorsAndWarningsAfterDelayTimeoutID !== null) { clearTimeout(flushPendingErrorsAndWarningsAfterDelayTimeoutID); } + } + + function flushPendingErrorsAndWarningsAfterDelay() { + clearPendingErrorsAndWarningsAfterDelay(); flushPendingErrorsAndWarningsAfterDelayTimeoutID = setTimeout(() => { flushPendingErrorsAndWarningsAfterDelayTimeoutID = null; if (pendingOperations.length > 0) { - // On the off chance that somethign else has pushed pending operations, + // 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; } @@ -1243,37 +1259,26 @@ export function attach( recordPendingErrorsAndWarnings(); if (pendingOperations.length === 0) { - // No warnings or errors to flush. + // No warnings or errors to flush; we can bail out early here too. return; } - const operations = new Array(2 + 1 + pendingOperations.length); - - // Identify which renderer this update is coming from. - // This enables roots to be mapped to renderers, - // 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++] = 0; // String table size + // 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; // Use this ID in case the root was unmounted! + operations[2] = 0; // String table size // Fill in the rest of the operations. for (let j = 0; j < pendingOperations.length; j++) { - operations[i + j] = pendingOperations[j]; + operations[3 + j] = pendingOperations[j]; } - // 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); pendingOperations.length = 0; }, 1000); @@ -1291,10 +1296,7 @@ export function attach( } function recordPendingErrorsAndWarnings() { - if (flushPendingErrorsAndWarningsAfterDelayTimeoutID !== null) { - clearTimeout(flushPendingErrorsAndWarningsAfterDelayTimeoutID); - flushPendingErrorsAndWarningsAfterDelayTimeoutID = null; - } + clearPendingErrorsAndWarningsAfterDelay(); fibersWithChangedErrorOrWarningCounts.forEach(fiberID => { const fiber = idToFiberMap.get(fiberID); @@ -1432,18 +1434,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;