From 794ed495a9cf6b9045f1263a27e592eda5a16fc9 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Wed, 19 May 2021 22:24:13 -0400 Subject: [PATCH 1/2] DevTools: Fix another Fast Refresh edge case Don't untrack unmounted Fibers after logging errors/warnings because it sometimes causes prematurely untracking a force-remounted Fiber from Fast Refresh, which in turn causes DevTools to break when the component is later inspected. This is a hack but I'm not sure of a better workaround. --- .../react-devtools-shared/src/__tests__/treeContext-test.js | 2 +- packages/react-devtools-shared/src/backend/renderer.js | 5 ++++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/treeContext-test.js b/packages/react-devtools-shared/src/__tests__/treeContext-test.js index 36f615c1155b6..8427dc9ca7fff 100644 --- a/packages/react-devtools-shared/src/__tests__/treeContext-test.js +++ b/packages/react-devtools-shared/src/__tests__/treeContext-test.js @@ -2384,7 +2384,7 @@ describe('TreeListContext', () => { ); expect(state).toMatchInlineSnapshot(` - ✕ 0, ⚠ 1 + ✕ 0, ⚠ 2 [root] ▾ ⚠ diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index c1f08846fdb1c..0505a9b550e90 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -1516,7 +1516,10 @@ export function attach( // We may also need to clean up after ourselves to avoid leaks. // See inline comments in onErrorOrWarning() for more info. if (isFiberMountedImpl(fiber) !== MOUNTED) { - untrackFiberID(fiber); + // HACK Cleaning up "unmounted" Fibers here can cause problems with Fast Refresh. + // Since warnings and errors are generally used in DEV mode, + // it may be better to ignore this kind of potential leak case rather than break Fast Refresh. + // untrackFiberID(fiber); return; } From 9abfcaad7b5eb7b0011b66bac9bc166be8f482f7 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Thu, 20 May 2021 11:17:08 -0400 Subject: [PATCH 2/2] Refactored DevTools pending error and warning mapping This way we avoid pre-allocating Fiber IDs for not-yet-mounted Fibers. --- .../src/__tests__/inspectedElement-test.js | 26 +-- .../src/__tests__/treeContext-test.js | 2 +- .../src/backend/renderer.js | 220 +++++++++++------- 3 files changed, 151 insertions(+), 97 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js index 1acc8a67c5db0..0c4fc869764e7 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js @@ -1946,15 +1946,11 @@ describe('InspectedElement', () => { }); describe('inline errors and warnings', () => { - // Some actions require the Fiber id. - // In those instances you might want to make assertions based on the ID instead of the index. - function getErrorsAndWarningsForElement(id: number) { - const index = ((store.getIndexOfElementID(id): any): number); - return getErrorsAndWarningsForElementAtIndex(index); - } - async function getErrorsAndWarningsForElementAtIndex(index) { const id = ((store.getElementIDAtIndex(index): any): number); + if (id == null) { + throw Error(`Element at index "${index}"" not found in store`); + } let errors = null; let warnings = null; @@ -2222,8 +2218,8 @@ describe('InspectedElement', () => { jest.runOnlyPendingTimers(); let data = [ - await getErrorsAndWarningsForElement(1), - await getErrorsAndWarningsForElement(2), + await getErrorsAndWarningsForElementAtIndex(0), + await getErrorsAndWarningsForElementAtIndex(1), ]; expect(data).toMatchInlineSnapshot(` Array [ @@ -2260,8 +2256,8 @@ describe('InspectedElement', () => { jest.runOnlyPendingTimers(); data = [ - await getErrorsAndWarningsForElement(1), - await getErrorsAndWarningsForElement(2), + await getErrorsAndWarningsForElementAtIndex(0), + await getErrorsAndWarningsForElementAtIndex(1), ]; expect(data).toMatchInlineSnapshot(` Array [ @@ -2319,8 +2315,8 @@ describe('InspectedElement', () => { jest.runOnlyPendingTimers(); let data = [ - await getErrorsAndWarningsForElement(1), - await getErrorsAndWarningsForElement(2), + await getErrorsAndWarningsForElementAtIndex(0), + await getErrorsAndWarningsForElementAtIndex(1), ]; expect(data).toMatchInlineSnapshot(` Array [ @@ -2357,8 +2353,8 @@ describe('InspectedElement', () => { jest.runOnlyPendingTimers(); data = [ - await getErrorsAndWarningsForElement(1), - await getErrorsAndWarningsForElement(2), + await getErrorsAndWarningsForElementAtIndex(0), + await getErrorsAndWarningsForElementAtIndex(1), ]; expect(data).toMatchInlineSnapshot(` Array [ diff --git a/packages/react-devtools-shared/src/__tests__/treeContext-test.js b/packages/react-devtools-shared/src/__tests__/treeContext-test.js index 8427dc9ca7fff..36f615c1155b6 100644 --- a/packages/react-devtools-shared/src/__tests__/treeContext-test.js +++ b/packages/react-devtools-shared/src/__tests__/treeContext-test.js @@ -2384,7 +2384,7 @@ describe('TreeListContext', () => { ); expect(state).toMatchInlineSnapshot(` - ✕ 0, ⚠ 2 + ✕ 0, ⚠ 1 [root] ▾ ⚠ diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 0505a9b550e90..da007e336cfff 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -564,50 +564,82 @@ export function attach( typeof setSuspenseHandler === 'function' && typeof scheduleUpdate === 'function'; - // Set of Fibers (IDs) with recently changed number of error/warning messages. - const fibersWithChangedErrorOrWarningCounts: Set = new Set(); + // Tracks Fibers with recently changed number of error/warning messages. + // These collections store the Fiber rather than the ID, + // in order to avoid generating an ID for Fibers that never get mounted + // (due to e.g. Suspense or error boundaries). + // onErrorOrWarning() adds Fibers and recordPendingErrorsAndWarnings() later clears them. + const fibersWithChangedErrorOrWarningCounts: Set = new Set(); + const pendingFiberToErrorsMap: Map> = new Map(); + const pendingFiberToWarningsMap: Map> = new Map(); // Mapping of fiber IDs to error/warning messages and counts. - const fiberToErrorsMap: Map> = new Map(); - const fiberToWarningsMap: Map> = new Map(); + const fiberIDToErrorsMap: Map> = new Map(); + const fiberIDToWarningsMap: Map> = new Map(); function clearErrorsAndWarnings() { // eslint-disable-next-line no-for-of-loops/no-for-of-loops - for (const id of fiberToErrorsMap.keys()) { - fibersWithChangedErrorOrWarningCounts.add(id); - updateMostRecentlyInspectedElementIfNecessary(id); + for (const id of fiberIDToErrorsMap.keys()) { + const fiber = idToArbitraryFiberMap.get(id); + if (fiber != null) { + fibersWithChangedErrorOrWarningCounts.add(fiber); + updateMostRecentlyInspectedElementIfNecessary(id); + } } // eslint-disable-next-line no-for-of-loops/no-for-of-loops - for (const id of fiberToWarningsMap.keys()) { - fibersWithChangedErrorOrWarningCounts.add(id); - updateMostRecentlyInspectedElementIfNecessary(id); + for (const id of fiberIDToWarningsMap.keys()) { + const fiber = idToArbitraryFiberMap.get(id); + if (fiber != null) { + fibersWithChangedErrorOrWarningCounts.add(fiber); + updateMostRecentlyInspectedElementIfNecessary(id); + } } - fiberToErrorsMap.clear(); - fiberToWarningsMap.clear(); + fiberIDToErrorsMap.clear(); + fiberIDToWarningsMap.clear(); flushPendingEvents(); } - function clearErrorsForFiberID(id: number) { - if (fiberToErrorsMap.has(id)) { - fiberToErrorsMap.delete(id); - fibersWithChangedErrorOrWarningCounts.add(id); - flushPendingEvents(); - } + function clearMessageCountHelper( + fiberID: number, + pendingFiberToMessageCountMap: Map>, + fiberIDToMessageCountMap: Map>, + ) { + const fiber = idToArbitraryFiberMap.get(fiberID); + if (fiber != null) { + // Throw out any pending changes. + pendingFiberToErrorsMap.delete(fiber); - updateMostRecentlyInspectedElementIfNecessary(id); - } + if (fiberIDToMessageCountMap.has(fiberID)) { + fiberIDToMessageCountMap.delete(fiberID); + + // If previous flushed counts have changed, schedule an update too. + fibersWithChangedErrorOrWarningCounts.add(fiber); + flushPendingEvents(); - function clearWarningsForFiberID(id: number) { - if (fiberToWarningsMap.has(id)) { - fiberToWarningsMap.delete(id); - fibersWithChangedErrorOrWarningCounts.add(id); - flushPendingEvents(); + updateMostRecentlyInspectedElementIfNecessary(fiberID); + } else { + fibersWithChangedErrorOrWarningCounts.delete(fiber); + } } + } - updateMostRecentlyInspectedElementIfNecessary(id); + function clearErrorsForFiberID(fiberID: number) { + clearMessageCountHelper( + fiberID, + pendingFiberToErrorsMap, + fiberIDToErrorsMap, + ); + } + + function clearWarningsForFiberID(fiberID: number) { + clearMessageCountHelper( + fiberID, + pendingFiberToWarningsMap, + fiberIDToWarningsMap, + ); } function updateMostRecentlyInspectedElementIfNecessary( @@ -628,36 +660,24 @@ export function attach( args: $ReadOnlyArray, ): void { const message = format(...args); - - // Note that by calling these functions we may be creating the ID for the first time. - // If the Fiber is then never mounted, we are responsible for cleaning up after ourselves. - // This is important because getOrGenerateFiberID() stores a Fiber in a couple of local Maps. - // If the Fiber never mounts and we don't clean up after this code, we could leak. - // Fortunately we would only leak Fibers that have errors/warnings associated with them, - // which is hopefully only a small set and only in DEV mode– but this is still not great. - // We should clean up Fibers like this when flushing; see recordPendingErrorsAndWarnings(). - const fiberID = getOrGenerateFiberID(fiber); - if (__DEBUG__) { debug('onErrorOrWarning', fiber, null, `${type}: "${message}"`); } // Mark this Fiber as needed its warning/error count updated during the next flush. - fibersWithChangedErrorOrWarningCounts.add(fiberID); + fibersWithChangedErrorOrWarningCounts.add(fiber); - // Update the error/warning messages and counts for the Fiber. - const fiberMap = type === 'error' ? fiberToErrorsMap : fiberToWarningsMap; - const messageMap = fiberMap.get(fiberID); + // Track the warning/error for later. + const fiberMap = + type === 'error' ? pendingFiberToErrorsMap : pendingFiberToWarningsMap; + const messageMap = fiberMap.get(fiber); if (messageMap != null) { const count = messageMap.get(message) || 0; messageMap.set(message, count + 1); } else { - fiberMap.set(fiberID, new Map([[message, 1]])); + fiberMap.set(fiber, new Map([[message, 1]])); } - // 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. @@ -1497,56 +1517,94 @@ export function attach( function reevaluateErrorsAndWarnings() { fibersWithChangedErrorOrWarningCounts.clear(); - fiberToErrorsMap.forEach((countMap, fiberID) => { - fibersWithChangedErrorOrWarningCounts.add(fiberID); + fiberIDToErrorsMap.forEach((countMap, fiberID) => { + const fiber = idToArbitraryFiberMap.get(fiberID); + if (fiber != null) { + fibersWithChangedErrorOrWarningCounts.add(fiber); + } }); - fiberToWarningsMap.forEach((countMap, fiberID) => { - fibersWithChangedErrorOrWarningCounts.add(fiberID); + fiberIDToWarningsMap.forEach((countMap, fiberID) => { + const fiber = idToArbitraryFiberMap.get(fiberID); + if (fiber != null) { + fibersWithChangedErrorOrWarningCounts.add(fiber); + } }); recordPendingErrorsAndWarnings(); } - function recordPendingErrorsAndWarnings() { - clearPendingErrorsAndWarningsAfterDelay(); + function mergeMapsAndGetCountHelper( + fiber: Fiber, + fiberID: number, + pendingFiberToMessageCountMap: Map>, + fiberIDToMessageCountMap: Map>, + ): number { + let newCount = 0; - fibersWithChangedErrorOrWarningCounts.forEach(fiberID => { - const fiber = idToArbitraryFiberMap.get(fiberID); - if (fiber != null) { - // Don't send updates for Fibers that didn't mount due to e.g. Suspense or an error boundary. - // We may also need to clean up after ourselves to avoid leaks. - // See inline comments in onErrorOrWarning() for more info. - if (isFiberMountedImpl(fiber) !== MOUNTED) { - // HACK Cleaning up "unmounted" Fibers here can cause problems with Fast Refresh. - // Since warnings and errors are generally used in DEV mode, - // it may be better to ignore this kind of potential leak case rather than break Fast Refresh. - // untrackFiberID(fiber); - return; - } + let messageCountMap = fiberIDToMessageCountMap.get(fiberID); - let errorCount = 0; - let warningCount = 0; + const pendingMessageCountMap = pendingFiberToMessageCountMap.get(fiber); + if (pendingMessageCountMap != null) { + if (messageCountMap == null) { + messageCountMap = pendingMessageCountMap; - if (!shouldFilterFiber(fiber)) { - const errorCountsMap = fiberToErrorsMap.get(fiberID); - const warningCountsMap = fiberToWarningsMap.get(fiberID); + fiberIDToMessageCountMap.set(fiberID, pendingMessageCountMap); + } else { + // This Flow refinement should not be necessary and yet... + const refinedMessageCountMap = ((messageCountMap: any): Map< + string, + number, + >); + + pendingMessageCountMap.forEach((pendingCount, message) => { + const previousCount = refinedMessageCountMap.get(message) || 0; + refinedMessageCountMap.set(message, previousCount + pendingCount); + }); + } + } - if (errorCountsMap != null) { - errorCountsMap.forEach(count => { - errorCount += count; - }); - } - if (warningCountsMap != null) { - warningCountsMap.forEach(count => { - warningCount += count; - }); - } - } + if (!shouldFilterFiber(fiber)) { + if (messageCountMap != null) { + messageCountMap.forEach(count => { + newCount += count; + }); + } + } + + pendingFiberToMessageCountMap.delete(fiber); + + return newCount; + } + + function recordPendingErrorsAndWarnings() { + clearPendingErrorsAndWarningsAfterDelay(); + + fibersWithChangedErrorOrWarningCounts.forEach(fiber => { + const fiberID = getFiberIDUnsafe(fiber); + if (fiberID === null) { + // Don't send updates for Fibers that didn't mount due to e.g. Suspense or an error boundary. + } else { + const errorCount = mergeMapsAndGetCountHelper( + fiber, + fiberID, + pendingFiberToErrorsMap, + fiberIDToErrorsMap, + ); + const warningCount = mergeMapsAndGetCountHelper( + fiber, + fiberID, + pendingFiberToWarningsMap, + fiberIDToWarningsMap, + ); pushOperation(TREE_OPERATION_UPDATE_ERRORS_OR_WARNINGS); pushOperation(fiberID); pushOperation(errorCount); pushOperation(warningCount); } + + // Always clean up so that we don't leak. + pendingFiberToErrorsMap.delete(fiber); + pendingFiberToWarningsMap.delete(fiber); }); fibersWithChangedErrorOrWarningCounts.clear(); } @@ -3015,8 +3073,8 @@ export function attach( rootType = fiberRoot._debugRootType; } - const errors = fiberToErrorsMap.get(id) || new Map(); - const warnings = fiberToWarningsMap.get(id) || new Map(); + const errors = fiberIDToErrorsMap.get(id) || new Map(); + const warnings = fiberIDToWarningsMap.get(id) || new Map(); return { id,