From 0240224d500b6616f12df91c62989940da98ee8c Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 4 Mar 2021 16:46:24 -0500 Subject: [PATCH 1/3] DevTools flushes updated passive warning/error info after delay Previously this information was not flushed until the next commit, but this provides a worse user experience if the next commit is really delayed. Instead, the backend now flushes only the warning/error counts after a delay. As a safety, if there are already any pending operations in the queue, we bail. --- .../src/__tests__/store-test.js | 23 ++++--- .../src/backend/renderer.js | 66 +++++++++++++++++++ 2 files changed, 77 insertions(+), 12 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index d5b9c3cbc9213..d852524a7f699 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -1075,10 +1075,7 @@ 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)', () => { + it('during passive get counted (after a delay)', () => { function Example() { React.useEffect(() => { console.error('test-only: passive error'); @@ -1089,18 +1086,20 @@ describe('Store', () => { const container = document.createElement('div'); withErrorsOrWarningsIgnored(['test-only:'], () => { - act(() => ReactDOM.render(, container)); - }); + act(() => { + ReactDOM.render(, container); - expect(store).toMatchInlineSnapshot(` - [root] - - `); + // Flush commit + jest.advanceTimersByTime(1); - withErrorsOrWarningsIgnored(['test-only:'], () => { - act(() => ReactDOM.render(, container)); + expect(store).toMatchInlineSnapshot(` + [root] + + `); + }); }); + // After a delay, passive effects should be committed as well expect(store).toMatchInlineSnapshot(` ✕ 1, ⚠ 1 [root] diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index c5f6932234a81..6a4e4d30549f5 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -626,6 +626,13 @@ 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 rendering might stil be going on. + flushPendingErrorsAndWarningsAfterDelay(); } // Patching the console enables DevTools to do a few useful things: @@ -1218,6 +1225,60 @@ export function attach( pendingOperations.push(op); } + let flushPendingErrorsAndWarningsAfterDelayTimeoutID = null; + function flushPendingErrorsAndWarningsAfterDelay() { + if (flushPendingErrorsAndWarningsAfterDelayTimeoutID !== null) { + clearTimeout(flushPendingErrorsAndWarningsAfterDelayTimeoutID); + } + + flushPendingErrorsAndWarningsAfterDelayTimeoutID = setTimeout(() => { + flushPendingErrorsAndWarningsAfterDelayTimeoutID = null; + + if (pendingOperations.length > 0) { + // On the off chance that somethign 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. + 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 + + // Fill in the rest of the operations. + for (let j = 0; j < pendingOperations.length; j++) { + operations[i + 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); + } + + pendingOperations.length = 0; + }, 1000); + } + function reevaluateErrorsAndWarnings() { fibersWithChangedErrorOrWarningCounts.clear(); fiberToErrorsMap.forEach((countMap, fiberID) => { @@ -1230,6 +1291,11 @@ export function attach( } function recordPendingErrorsAndWarnings() { + if (flushPendingErrorsAndWarningsAfterDelayTimeoutID !== null) { + clearTimeout(flushPendingErrorsAndWarningsAfterDelayTimeoutID); + flushPendingErrorsAndWarningsAfterDelayTimeoutID = null; + } + fibersWithChangedErrorOrWarningCounts.forEach(fiberID => { const fiber = idToFiberMap.get(fiberID); if (fiber != null) { From b7866cfafea73ca562dff297f190e4f6d34d0064 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Fri, 5 Mar 2021 10:20:16 +0100 Subject: [PATCH 2/3] Make timing explicit in test --- .../src/__tests__/store-test.js | 20 +++++++++++-------- .../src/__tests__/utils.js | 19 +++++++++++------- 2 files changed, 24 insertions(+), 15 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index d852524a7f699..d1a04b74b5b9d 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -1088,16 +1088,20 @@ describe('Store', () => { withErrorsOrWarningsIgnored(['test-only:'], () => { act(() => { ReactDOM.render(, container); + // flush bridge operations + jest.runOnlyPendingTimers(); + }, false); + }); - // Flush commit - jest.advanceTimersByTime(1); + expect(store).toMatchInlineSnapshot(` + [root] + + `); - expect(store).toMatchInlineSnapshot(` - [root] - - `); - }); - }); + // flush count after delay + act(() => { + jest.advanceTimersByTime(1000); + }, false); // After a delay, passive effects should be committed as well expect(store).toMatchInlineSnapshot(` 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(); + }); }); - }); + } } } From 877037f176721bc7ad363e9667c7bd5c48420d7b Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Fri, 5 Mar 2021 10:03:22 -0500 Subject: [PATCH 3/3] Cleaned up renderer code and added additional unit test --- .../src/__tests__/store-test.js | 131 +++++++++++++----- .../src/backend/renderer.js | 84 +++++------ 2 files changed, 137 insertions(+), 78 deletions(-) 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..b23b59f73d84a 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,31 @@ 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); + flushPendingErrorsAndWarningsAfterDelayTimeoutID = null; } + } + + 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 +1260,24 @@ 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 - - // Fill in the rest of the operations. + // 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[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 +1295,7 @@ export function attach( } function recordPendingErrorsAndWarnings() { - if (flushPendingErrorsAndWarningsAfterDelayTimeoutID !== null) { - clearTimeout(flushPendingErrorsAndWarningsAfterDelayTimeoutID); - flushPendingErrorsAndWarningsAfterDelayTimeoutID = null; - } + clearPendingErrorsAndWarningsAfterDelay(); fibersWithChangedErrorOrWarningCounts.forEach(fiberID => { const fiber = idToFiberMap.get(fiberID); @@ -1385,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, ...] @@ -1432,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;