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 35bc0ab0f9153..189da54c8603b 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -157,8 +157,7 @@ type FiberInstance = { firstChild: null | DevToolsInstance, nextSibling: null | DevToolsInstance, source: null | string | Error | Source, // source location of this component function, or owned child stack - errors: null | Map, // error messages and count - warnings: null | Map, // warning messages and count + logCount: number, // total number of errors/warnings last seen treeBaseDuration: number, // the profiled time of the last render of this subtree data: Fiber, // one of a Fiber pair }; @@ -171,8 +170,7 @@ function createFiberInstance(fiber: Fiber): FiberInstance { firstChild: null, nextSibling: null, source: null, - errors: null, - warnings: null, + logCount: 0, treeBaseDuration: 0, data: fiber, }; @@ -187,8 +185,7 @@ type FilteredFiberInstance = { firstChild: null | DevToolsInstance, nextSibling: null | DevToolsInstance, source: null | string | Error | Source, // always null here. - errors: null, // error messages and count - warnings: null, // warning messages and count + logCount: number, // total number of errors/warnings last seen treeBaseDuration: number, // the profiled time of the last render of this subtree data: Fiber, // one of a Fiber pair }; @@ -201,9 +198,8 @@ function createFilteredFiberInstance(fiber: Fiber): FilteredFiberInstance { parent: null, firstChild: null, nextSibling: null, - componentStack: null, - errors: null, - warnings: null, + source: null, + logCount: 0, treeBaseDuration: 0, data: fiber, }: any); @@ -221,11 +217,7 @@ type VirtualInstance = { firstChild: null | DevToolsInstance, nextSibling: null | DevToolsInstance, source: null | string | Error | Source, // source location of this server component, or owned child stack - // Errors and Warnings happen per ReactComponentInfo which can appear in - // multiple places but we track them per stateful VirtualInstance so - // that old errors/warnings don't disappear when the instance is refreshed. - errors: null | Map, // error messages and count - warnings: null | Map, // warning messages and count + logCount: number, // total number of errors/warnings last seen treeBaseDuration: number, // the profiled time of the last render of this subtree // The latest info for this instance. This can be updated over time and the // same info can appear in more than once ServerComponentInstance. @@ -242,8 +234,7 @@ function createVirtualInstance( firstChild: null, nextSibling: null, source: null, - errors: null, - warnings: null, + logCount: 0, treeBaseDuration: 0, data: debugEntry, }; @@ -968,74 +959,91 @@ export function attach( toggleProfilingStatus = response.toggleProfilingStatus; } - // Tracks Fibers with recently changed number of error/warning messages. - // These collections store the Fiber rather than the DevToolsInstance, - // in order to avoid generating an DevToolsInstance 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: WeakMap< - Fiber, - Map, - > = new WeakMap(); - const pendingFiberToWarningsMap: WeakMap< - Fiber, - Map, - > = new WeakMap(); + type ComponentLogs = { + errors: Map, + errorsCount: number, + warnings: Map, + warningsCount: number, + }; + // Tracks Errors/Warnings logs added to a Fiber. They are added before the commit and get + // picked up a FiberInstance. This keeps it around as long as the Fiber is alive which + // 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 + // and then mount, the logs are there. Ensuring we only clear what you've seen. + // If we wanted to clear the whole set, we'd replace fiberToComponentLogsMap with a + // new WeakMap. + // eslint-disable-next-line no-for-of-loops/no-for-of-loops for (const devtoolsInstance of idToDevToolsInstanceMap.values()) { - devtoolsInstance.errors = null; - devtoolsInstance.warnings = null; if (devtoolsInstance.kind === FIBER_INSTANCE) { - fibersWithChangedErrorOrWarningCounts.add(devtoolsInstance.data); + const fiber = devtoolsInstance.data; + fiberToComponentLogsMap.delete(fiber); + if (fiber.alternate) { + fiberToComponentLogsMap.delete(fiber.alternate); + } } else { // TODO: Handle VirtualInstance. } - updateMostRecentlyInspectedElementIfNecessary(devtoolsInstance.id); + const changed = recordConsoleLogs(devtoolsInstance, undefined); + if (changed) { + updateMostRecentlyInspectedElementIfNecessary(devtoolsInstance.id); + } } flushPendingEvents(); } - function clearMessageCountHelper( - instanceID: number, - pendingFiberToMessageCountMap: WeakMap>, - forError: boolean, - ) { + function clearConsoleLogsHelper(instanceID: number, type: 'error' | 'warn') { const devtoolsInstance = idToDevToolsInstanceMap.get(instanceID); if (devtoolsInstance !== undefined) { - let changed = false; - if (forError) { - if ( - devtoolsInstance.errors !== null && - devtoolsInstance.errors.size > 0 - ) { - changed = true; - } - devtoolsInstance.errors = null; - } else { - if ( - devtoolsInstance.warnings !== null && - devtoolsInstance.warnings.size > 0 - ) { - changed = true; - } - devtoolsInstance.warnings = null; - } if (devtoolsInstance.kind === FIBER_INSTANCE) { const fiber = devtoolsInstance.data; - // Throw out any pending changes. - pendingFiberToMessageCountMap.delete(fiber); - - if (changed) { - // If previous flushed counts have changed, schedule an update too. - fibersWithChangedErrorOrWarningCounts.add(fiber); - flushPendingEvents(); - - updateMostRecentlyInspectedElementIfNecessary(instanceID); - } else { - fibersWithChangedErrorOrWarningCounts.delete(fiber); + const componentLogsEntry = fiberToComponentLogsMap.get(fiber); + if (componentLogsEntry !== undefined) { + if (type === 'error') { + componentLogsEntry.errors.clear(); + componentLogsEntry.errorsCount = 0; + } else { + componentLogsEntry.warnings.clear(); + componentLogsEntry.warningsCount = 0; + } + const changed = recordConsoleLogs( + devtoolsInstance, + componentLogsEntry, + ); + if (changed) { + flushPendingEvents(); + updateMostRecentlyInspectedElementIfNecessary(devtoolsInstance.id); + } } } else { // TODO: Handle VirtualInstance. @@ -1044,11 +1052,11 @@ export function attach( } function clearErrorsForElementID(instanceID: number) { - clearMessageCountHelper(instanceID, pendingFiberToErrorsMap, true); + clearConsoleLogsHelper(instanceID, 'error'); } function clearWarningsForElementID(instanceID: number) { - clearMessageCountHelper(instanceID, pendingFiberToWarningsMap, false); + clearConsoleLogsHelper(instanceID, 'warn'); } function updateMostRecentlyInspectedElementIfNecessary( @@ -1086,34 +1094,45 @@ export function attach( // [Warning: %o, {...}] and [Warning: %o, {...}] will be considered as the same message, // even if objects are different const message = formatConsoleArgumentsToSingleString(...args); - if (__DEBUG__) { - const fiberInstance = fiberToFiberInstanceMap.get(fiber); - if (fiberInstance !== undefined) { - debug('onErrorOrWarning', fiberInstance, null, `${type}: "${message}"`); - } - } - - // Mark this Fiber as needed its warning/error count updated during the next flush. - fibersWithChangedErrorOrWarningCounts.add(fiber); // 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); + let componentLogsEntry = fiberToComponentLogsMap.get(fiber); + if (componentLogsEntry === undefined && fiber.alternate !== null) { + componentLogsEntry = fiberToComponentLogsMap.get(fiber.alternate); + if (componentLogsEntry !== undefined) { + // Use the same set for both Fibers. + fiberToComponentLogsMap.set(fiber, componentLogsEntry); + } + } + if (componentLogsEntry === undefined) { + componentLogsEntry = { + errors: new Map(), + errorsCount: 0, + warnings: new Map(), + warningsCount: 0, + }; + fiberToComponentLogsMap.set(fiber, componentLogsEntry); + } + + const messageMap = + type === 'error' + ? componentLogsEntry.errors + : componentLogsEntry.warnings; + const count = messageMap.get(message) || 0; + messageMap.set(message, count + 1); + if (type === 'error') { + componentLogsEntry.errorsCount++; } else { - fiberMap.set(fiber, new Map([[message, 1]])); + componentLogsEntry.warningsCount++; } - // 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(); + // The changes will be flushed later when we commit. + + // 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: @@ -1330,9 +1349,9 @@ export function attach( currentRoot = (null: any); }); - // Also re-evaluate all error and warning counts given the new filters. - reevaluateErrorsAndWarnings(); flushPendingEvents(); + + needsToFlushComponentLogs = false; } function getEnvironmentNames(): Array { @@ -1530,29 +1549,6 @@ export function attach( // When a mount or update is in progress, this value tracks the root that is being operated on. let currentRoot: FiberInstance = (null: any); - // Returns a FiberInstance if one has already been generated for the Fiber or null if one has not been generated. - // Use this method while e.g. logging to avoid over-retaining Fibers. - function getFiberInstanceUnsafe(fiber: Fiber): FiberInstance | null { - const fiberInstance = fiberToFiberInstanceMap.get(fiber); - if (fiberInstance !== undefined) { - return fiberInstance; - } else { - const {alternate} = fiber; - if (alternate !== null) { - const alternateInstance = fiberToFiberInstanceMap.get(alternate); - if (alternateInstance !== undefined) { - return alternateInstance; - } - } - } - return null; - } - - function getFiberIDUnsafe(fiber: Fiber): number | null { - const fiberInstance = getFiberInstanceUnsafe(fiber); - return fiberInstance === null ? null : fiberInstance.id; - } - // Removes a Fiber (and its alternate) from the Maps used to track their id. // This method should always be called when a Fiber is unmounting. function untrackFiber(nearestInstance: DevToolsInstance, fiber: Fiber) { @@ -1850,164 +1846,40 @@ export function attach( } } - let flushPendingErrorsAndWarningsAfterDelayTimeoutID: null | TimeoutID = 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 (shouldBailoutWithPendingOperations()) { - // 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; - if (currentRoot === null) { - // TODO: This is not always safe so this field is probably not needed. - operations[1] = -1; - } else { - operations[1] = currentRoot.id; - } - 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(); - // eslint-disable-next-line no-for-of-loops/no-for-of-loops - for (const devtoolsInstance of idToDevToolsInstanceMap.values()) { - if (devtoolsInstance.kind === FIBER_INSTANCE) { - fibersWithChangedErrorOrWarningCounts.add(devtoolsInstance.data); - } else { - // TODO: Handle VirtualInstance. - } - } - recordPendingErrorsAndWarnings(); - } - - function mergeMapsAndGetCountHelper( - fiber: Fiber, - fiberID: number, - pendingFiberToMessageCountMap: WeakMap>, - forError: boolean, - ): number { - let newCount = 0; - - const devtoolsInstance = idToDevToolsInstanceMap.get(fiberID); - - if (devtoolsInstance === undefined) { - return 0; - } - - let messageCountMap = forError - ? devtoolsInstance.errors - : devtoolsInstance.warnings; - - const pendingMessageCountMap = pendingFiberToMessageCountMap.get(fiber); - if (pendingMessageCountMap != null) { - if (messageCountMap === null) { - messageCountMap = pendingMessageCountMap; - if (forError) { - devtoolsInstance.errors = pendingMessageCountMap; - } else { - devtoolsInstance.warnings = 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); - }); + function recordConsoleLogs( + instance: FiberInstance | VirtualInstance, + componentLogsEntry: void | ComponentLogs, + ): boolean { + if (componentLogsEntry === undefined) { + if (instance.logCount === 0) { + // Nothing has changed. + return false; } - } - - if (!shouldFilterFiber(fiber)) { - if (messageCountMap != null) { - messageCountMap.forEach(count => { - newCount += count; - }); + // Reset to zero. + instance.logCount = 0; + pushOperation(TREE_OPERATION_UPDATE_ERRORS_OR_WARNINGS); + pushOperation(instance.id); + pushOperation(0); + pushOperation(0); + return true; + } else { + const totalCount = + componentLogsEntry.errorsCount + componentLogsEntry.warningsCount; + if (instance.logCount === totalCount) { + // Nothing has changed. + return false; } + // Update counts. + instance.logCount = totalCount; + pushOperation(TREE_OPERATION_UPDATE_ERRORS_OR_WARNINGS); + pushOperation(instance.id); + pushOperation(componentLogsEntry.errorsCount); + pushOperation(componentLogsEntry.warningsCount); + return true; } - - 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, - true, - ); - const warningCount = mergeMapsAndGetCountHelper( - fiber, - fiberID, - pendingFiberToWarningsMap, - false, - ); - - pushOperation(TREE_OPERATION_UPDATE_ERRORS_OR_WARNINGS); - pushOperation(fiberID); - pushOperation(errorCount); - pushOperation(warningCount); - - // Only clear the ones that we've already shown. Leave others in case - // they mount later. - pendingFiberToErrorsMap.delete(fiber); - pendingFiberToWarningsMap.delete(fiber); - } - }); - fibersWithChangedErrorOrWarningCounts.clear(); } function flushPendingEvents(root: Object): void { - // Add any pending errors and warnings to the operations array. - recordPendingErrorsAndWarnings(); - if (shouldBailoutWithPendingOperations()) { // If we aren't profiling, we can just bail out here. // No use sending an empty update over the bridge. @@ -2260,6 +2132,12 @@ export function attach( } } + let componentLogsEntry = fiberToComponentLogsMap.get(fiber); + if (componentLogsEntry === undefined && fiber.alternate !== null) { + componentLogsEntry = fiberToComponentLogsMap.get(fiber.alternate); + } + recordConsoleLogs(fiberInstance, componentLogsEntry); + if (isProfilingSupported) { recordProfilingDurations(fiberInstance, null); } @@ -2359,19 +2237,6 @@ export function attach( idToDevToolsInstanceMap.delete(fiberInstance.id); - // Restore any errors/warnings associated with this fiber to the pending - // map. I.e. treat it as before we tracked the instances. This lets us - // restore them if we remount the same Fibers later. Otherwise we rely - // on the GC of the Fibers to clean them up. - if (fiberInstance.errors !== null) { - pendingFiberToErrorsMap.set(fiber, fiberInstance.errors); - fiberInstance.errors = null; - } - if (fiberInstance.warnings !== null) { - pendingFiberToWarningsMap.set(fiber, fiberInstance.warnings); - fiberInstance.warnings = null; - } - if (fiberToFiberInstanceMap.get(fiber) === fiberInstance) { fiberToFiberInstanceMap.delete(fiber); } @@ -3515,6 +3380,16 @@ export function attach( } if (fiberInstance !== null) { + let componentLogsEntry = fiberToComponentLogsMap.get( + fiberInstance.data, + ); + if (componentLogsEntry === undefined && fiberInstance.data.alternate) { + componentLogsEntry = fiberToComponentLogsMap.get( + fiberInstance.data.alternate, + ); + } + recordConsoleLogs(fiberInstance, componentLogsEntry); + const isProfilingSupported = nextFiber.hasOwnProperty('treeBaseDuration'); if (isProfilingSupported) { @@ -3619,6 +3494,8 @@ export function attach( mountFiberRecursively(root.current, false); flushPendingEvents(root); + + needsToFlushComponentLogs = false; currentRoot = (null: any); }); } @@ -3641,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( @@ -3750,6 +3636,8 @@ export function attach( // We're done here. flushPendingEvents(root); + needsToFlushComponentLogs = false; + if (traceUpdatesEnabled) { hook.emit('traceUpdates', traceUpdatesForNodes); } @@ -4337,6 +4225,8 @@ export function attach( source = getSourceForFiberInstance(fiberInstance); } + const componentLogsEntry = fiberToComponentLogsMap.get(fiber); + return { id: fiberInstance.id, @@ -4387,13 +4277,13 @@ export function attach( props: memoizedProps, state: showState ? memoizedState : null, errors: - fiberInstance.errors === null + componentLogsEntry === undefined ? [] - : Array.from(fiberInstance.errors.entries()), + : Array.from(componentLogsEntry.errors.entries()), warnings: - fiberInstance.warnings === null + componentLogsEntry === undefined ? [] - : Array.from(fiberInstance.warnings.entries()), + : Array.from(componentLogsEntry.warnings.entries()), // List of owners owners, @@ -4478,15 +4368,8 @@ export function attach( hooks: null, props: props, state: null, - errors: - virtualInstance.errors === null - ? [] - : Array.from(virtualInstance.errors.entries()), - warnings: - virtualInstance.warnings === null - ? [] - : Array.from(virtualInstance.warnings.entries()), - + errors: [], // TODO: Handle errors on Virtual Instances. + warnings: [], // TODO: Handle warnings on Virtual Instances. // List of owners owners,