diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index e8a75e1d234a3..1fc02e2ad75d2 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -68,7 +68,7 @@ import { NoWork, computeAsyncExpirationNoBucket, } from './ReactFiberExpirationTime'; -import {onCommitUnmount} from './ReactFiberDevToolsHook'; +import {isDevToolsPresent, onCommitUnmount} from './ReactFiberDevToolsHook'; import {startPhaseTimer, stopPhaseTimer} from './ReactDebugFiberPerf'; import {getStackByFiberInDevAndProd} from './ReactCurrentFiber'; import {logCapturedError} from './ReactFiberErrorLogger'; @@ -1341,8 +1341,10 @@ function commitSuspenseComponent( retry = Schedule_tracing_wrap(retry); } if (enableUpdaterTracking) { - // If we have pending work still, restore the original updaters - restorePendingUpdaters(finishedRoot, committedExpirationTime); + if (isDevToolsPresent) { + // If we have pending work still, restore the original updaters + restorePendingUpdaters(finishedRoot, committedExpirationTime); + } } retryCache.add(thenable); thenable.then(retry, retry); diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index a9c4f5e800fc4..66f1764f2dec5 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -167,7 +167,7 @@ import { hasCaughtError, clearCaughtError, } from 'shared/ReactErrorUtils'; -import {onCommitRoot} from './ReactFiberDevToolsHook'; +import {isDevToolsPresent, onCommitRoot} from './ReactFiberDevToolsHook'; const ceil = Math.ceil; @@ -339,13 +339,15 @@ export function scheduleUpdateOnFiber( } if (enableUpdaterTracking) { - const pendingUpdatersMap = root.pendingUpdatersMap; - let updaters = pendingUpdatersMap.get(expirationTime); - if (updaters == null) { - updaters = new Set(); - pendingUpdatersMap.set(expirationTime, updaters); + if (isDevToolsPresent) { + const pendingUpdatersMap = root.pendingUpdatersMap; + let updaters = pendingUpdatersMap.get(expirationTime); + if (updaters == null) { + updaters = new Set(); + pendingUpdatersMap.set(expirationTime, updaters); + } + updaters.add(fiber); } - updaters.add(fiber); } root.pingTime = NoWork; @@ -813,6 +815,17 @@ function renderRoot( Interaction, >); } + + if (enableUpdaterTracking) { + if (isDevToolsPresent) { + // If we didn't finish the current work, restore pending updaters for the next pass. + if (root.memoizedUpdaters.size > 0) { + restorePendingUpdaters(root, expirationTime); + root.memoizedUpdaters.clear(); + } + } + } + return renderRoot.bind(null, root, currentTime); } } @@ -878,6 +891,17 @@ function renderRoot( if (expirationTime !== Sync) { startRequestCallbackTimer(); } + + if (enableUpdaterTracking) { + if (isDevToolsPresent) { + // If we didn't finish the current work, restore pending updaters for the next pass. + if (root.memoizedUpdaters.size > 0) { + restorePendingUpdaters(root, expirationTime); + root.memoizedUpdaters.clear(); + } + } + } + return renderRoot.bind(null, root, expirationTime); } } @@ -1305,8 +1329,10 @@ function commitRootImpl(root) { root.lastPendingTime = firstPendingTimeBeforeCommit; if (enableUpdaterTracking) { - if (firstPendingTimeBeforeCommit !== NoWork) { - restorePendingUpdaters(root, root.lastPendingTime); + if (isDevToolsPresent) { + if (firstPendingTimeBeforeCommit !== NoWork) { + restorePendingUpdaters(root, root.lastPendingTime); + } } } } @@ -1515,6 +1541,12 @@ function commitRootImpl(root) { onCommitRoot(finishedWork.stateNode); + if (enableUpdaterTracking) { + if (isDevToolsPresent) { + root.memoizedUpdaters.clear(); + } + } + if (remainingExpirationTime === Sync) { // Count the number of times the root synchronously re-renders without // finishing. If there are too many, it indicates an infinite update loop. @@ -2191,7 +2223,7 @@ export function restorePendingUpdaters( root: FiberRoot, expirationTime: ExpirationTime, ): void { - if (!enableUpdaterTracking) { + if (!enableUpdaterTracking || !isDevToolsPresent) { return; } const pendingUpdatersMap = root.pendingUpdatersMap; @@ -2323,20 +2355,21 @@ function startWorkOnPendingInteraction(root, expirationTime) { // This is called when new work is started on a root. if (enableUpdaterTracking) { - const memoizedUpdaters: Set = new Set(); - const pendingUpdatersMap = root.pendingUpdatersMap; - pendingUpdatersMap.forEach((updaters, scheduledExpirationTime) => { - if (scheduledExpirationTime >= expirationTime) { - pendingUpdatersMap.delete(scheduledExpirationTime); - updaters.forEach(fiber => memoizedUpdaters.add(fiber)); - } - }); - - // Store the current set of interactions on the FiberRoot for a few reasons: - // We can re-use it in hot functions like renderRoot() without having to - // recalculate it. This also provides DevTools with a way to access it when - // the onCommitRoot() hook is called. - root.memoizedUpdaters = memoizedUpdaters; + if (isDevToolsPresent) { + // Store the current set of interactions on the FiberRoot for a few reasons: + // We can re-use it in hot functions like renderRoot() without having to + // recalculate it. This also provides DevTools with a way to access it when + // the onCommitRoot() hook is called. + const pendingUpdatersMap = root.pendingUpdatersMap; + pendingUpdatersMap.forEach((updaters, scheduledExpirationTime) => { + if (scheduledExpirationTime >= expirationTime) { + pendingUpdatersMap.delete(scheduledExpirationTime); + updaters.forEach(fiber => { + root.memoizedUpdaters.add(fiber); + }); + } + }); + } } if (enableSchedulerTracing) { diff --git a/packages/react-reconciler/src/ReactFiberUnwindWork.js b/packages/react-reconciler/src/ReactFiberUnwindWork.js index 46a4eb042eaaa..3b57c446a911d 100644 --- a/packages/react-reconciler/src/ReactFiberUnwindWork.js +++ b/packages/react-reconciler/src/ReactFiberUnwindWork.js @@ -79,6 +79,7 @@ import { checkForWrongSuspensePriorityInDEV, restorePendingUpdaters, } from './ReactFiberScheduler'; +import {isDevToolsPresent} from './ReactFiberDevToolsHook'; import invariant from 'shared/invariant'; @@ -190,8 +191,10 @@ function attachPingListener( ping = Schedule_tracing_wrap(ping); } if (enableUpdaterTracking) { - // If we have pending work still, restore the original updaters - restorePendingUpdaters(root, renderExpirationTime); + if (isDevToolsPresent) { + // If we have pending work still, restore the original updaters + restorePendingUpdaters(root, renderExpirationTime); + } } thenable.then(ping, ping); } @@ -210,8 +213,10 @@ function throwException( sourceFiber.firstEffect = sourceFiber.lastEffect = null; if (enableUpdaterTracking) { - // If we have pending work still, restore the original updaters - restorePendingUpdaters(root, renderExpirationTime); + if (isDevToolsPresent) { + // If we have pending work still, restore the original updaters + restorePendingUpdaters(root, renderExpirationTime); + } } if ( diff --git a/packages/react-reconciler/src/__tests__/ReactUpdaters-test.internal.js b/packages/react-reconciler/src/__tests__/ReactUpdaters-test.internal.js index ea18b46a5587f..86523862481f7 100644 --- a/packages/react-reconciler/src/__tests__/ReactUpdaters-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactUpdaters-test.internal.js @@ -12,50 +12,62 @@ let React; let ReactFeatureFlags; let ReactDOM; +let ReactTestRenderer; +let ReactTestUtils; let Scheduler; -let TestUtils; let mockDevToolsHook; let allSchedulerTags; let allSchedulerTypes; +let onCommitRootShouldYield; -describe('updaters', () => { - beforeEach(() => { - jest.resetModules(); - - allSchedulerTags = []; - allSchedulerTypes = []; +function loadModules({useDOMRenderer = true} = {}) { + jest.resetModules(); - mockDevToolsHook = { - injectInternals: jest.fn(() => {}), - onCommitRoot: jest.fn(fiberRoot => { - Scheduler.yieldValue('onCommitRoot'); - const schedulerTags = []; - const schedulerTypes = []; - fiberRoot.memoizedUpdaters.forEach(fiber => { - schedulerTags.push(fiber.tag); - schedulerTypes.push(fiber.elementType); - }); - allSchedulerTags.push(schedulerTags); - allSchedulerTypes.push(schedulerTypes); - }), - onCommitUnmount: jest.fn(() => {}), - isDevToolsPresent: true, - }; + allSchedulerTags = []; + allSchedulerTypes = []; - jest.mock( - 'react-reconciler/src/ReactFiberDevToolsHook', - () => mockDevToolsHook, - ); + onCommitRootShouldYield = true; - ReactFeatureFlags = require('shared/ReactFeatureFlags'); - ReactFeatureFlags.enableUpdaterTracking = true; - ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; - - React = require('react'); + mockDevToolsHook = { + injectInternals: jest.fn(() => {}), + onCommitRoot: jest.fn(fiberRoot => { + if (onCommitRootShouldYield) { + Scheduler.yieldValue('onCommitRoot'); + } + const schedulerTags = []; + const schedulerTypes = []; + fiberRoot.memoizedUpdaters.forEach(fiber => { + schedulerTags.push(fiber.tag); + schedulerTypes.push(fiber.elementType); + }); + allSchedulerTags.push(schedulerTags); + allSchedulerTypes.push(schedulerTypes); + }), + onCommitUnmount: jest.fn(() => {}), + isDevToolsPresent: true, + }; + + jest.mock( + 'react-reconciler/src/ReactFiberDevToolsHook', + () => mockDevToolsHook, + ); + + ReactFeatureFlags = require('shared/ReactFeatureFlags'); + ReactFeatureFlags.enableUpdaterTracking = true; + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; + + React = require('react'); + if (useDOMRenderer) { ReactDOM = require('react-dom'); - Scheduler = require('scheduler'); - TestUtils = require('react-dom/test-utils'); - }); + ReactTestUtils = require('react-dom/test-utils'); + } else { + ReactTestRenderer = require('react-test-renderer'); + } + Scheduler = require('scheduler'); +} + +describe('updaters', () => { + beforeEach(() => loadModules()); it('should report the (host) root as the scheduler for root-level render', () => { const {HostRoot} = require('shared/ReactWorkTags'); @@ -64,14 +76,14 @@ describe('updaters', () => { const Child = () => null; const container = document.createElement('div'); - TestUtils.act(() => { + ReactTestUtils.act(() => { ReactDOM.render(, container); }); expect(allSchedulerTags).toHaveLength(1); expect(allSchedulerTags[0]).toHaveLength(1); expect(allSchedulerTags[0]).toContain(HostRoot); - TestUtils.act(() => { + ReactTestUtils.act(() => { ReactDOM.render(, container); }); expect(allSchedulerTags).toHaveLength(2); @@ -101,19 +113,19 @@ describe('updaters', () => { }; const Child = () => null; - TestUtils.act(() => { + ReactTestUtils.act(() => { ReactDOM.render(, document.createElement('div')); }); expect(scheduleForA).not.toBeNull(); expect(scheduleForB).not.toBeNull(); expect(allSchedulerTypes).toHaveLength(1); - TestUtils.act(scheduleForA); + ReactTestUtils.act(scheduleForA); expect(allSchedulerTypes).toHaveLength(2); expect(allSchedulerTypes[1]).toHaveLength(1); expect(allSchedulerTypes[1]).toContain(SchedulingComponentA); - TestUtils.act(scheduleForB); + ReactTestUtils.act(scheduleForB); expect(allSchedulerTypes).toHaveLength(3); expect(allSchedulerTypes[2]).toHaveLength(1); expect(allSchedulerTypes[2]).toContain(SchedulingComponentB); @@ -130,13 +142,13 @@ describe('updaters', () => { } const Child = () => null; let instance; - TestUtils.act(() => { + ReactTestUtils.act(() => { ReactDOM.render(, document.createElement('div')); }); expect(allSchedulerTypes).toHaveLength(1); expect(instance).not.toBeNull(); - TestUtils.act(() => { + ReactTestUtils.act(() => { instance.setState({}); }); expect(allSchedulerTypes).toHaveLength(2); @@ -180,7 +192,7 @@ describe('updaters', () => { }; const root = ReactDOM.unstable_createRoot(document.createElement('div')); - TestUtils.act(() => { + ReactTestUtils.act(() => { root.render(); expect(Scheduler).toFlushAndYieldThrough([ 'CascadingChild 0', @@ -191,7 +203,7 @@ describe('updaters', () => { expect(triggerPassiveCascade).not.toBeNull(); expect(allSchedulerTypes).toHaveLength(1); - TestUtils.act(() => { + ReactTestUtils.act(() => { triggerActiveCascade(); expect(Scheduler).toFlushAndYieldThrough([ 'CascadingChild 0', @@ -206,7 +218,7 @@ describe('updaters', () => { expect(allSchedulerTypes[2]).toHaveLength(1); expect(allSchedulerTypes[2]).toContain(CascadingChild); - TestUtils.act(() => { + ReactTestUtils.act(() => { triggerPassiveCascade(); expect(Scheduler).toFlushAndYieldThrough([ 'CascadingChild 1', @@ -259,14 +271,14 @@ describe('updaters', () => { } }; - TestUtils.act(() => { + ReactTestUtils.act(() => { ReactDOM.render(, document.createElement('div')); expect(Scheduler).toHaveYielded(['onCommitRoot']); }); expect(setShouldSuspend).not.toBeNull(); expect(allSchedulerTypes).toHaveLength(1); - TestUtils.act(() => { + ReactTestUtils.act(() => { setShouldSuspend(true); expect(Scheduler).toFlushAndYieldThrough(['onCommitRoot']); }); @@ -275,7 +287,7 @@ describe('updaters', () => { expect(allSchedulerTypes[1]).toContain(Suspender); expect(resolver).not.toBeNull(); - await TestUtils.act(() => { + await ReactTestUtils.act(() => { resolver('abc'); return promise; }); @@ -318,14 +330,14 @@ describe('updaters', () => { }; const root = ReactDOM.unstable_createRoot(document.createElement('div')); - TestUtils.act(() => { + ReactTestUtils.act(() => { root.render(); expect(Scheduler).toFlushAndYieldThrough(['NotHidden', 'onCommitRoot']); }); expect(allSchedulerTypes).toHaveLength(1); expect(setIncludeHiddenTree).not.toBeNull(); - TestUtils.act(() => { + ReactTestUtils.act(() => { setIncludeHiddenTree(true); expect(Scheduler).toFlushAndYieldThrough(['NotHidden', 'onCommitRoot']); expect(allSchedulerTypes).toHaveLength(2); @@ -381,7 +393,7 @@ describe('updaters', () => { }; const root = ReactDOM.unstable_createRoot(document.createElement('div')); - TestUtils.act(() => { + ReactTestUtils.act(() => { root.render(); expect(Scheduler).toFlushAndYieldThrough(['initial', 'onCommitRoot']); }); @@ -389,6 +401,7 @@ describe('updaters', () => { const schedulerTypes = []; + // TODO DELETE THIS mockDevToolsHook.onCommitRoot.mockImplementation(fiberRoot => { Scheduler.yieldValue('onCommitRoot'); schedulerTypes.push( @@ -396,7 +409,7 @@ describe('updaters', () => { ); }); - TestUtils.act(() => { + ReactTestUtils.act(() => { triggerError(); expect(Scheduler).toFlushAndYieldThrough([ 'onCommitRoot', @@ -436,7 +449,7 @@ describe('updaters', () => { }; const root = ReactDOM.unstable_createRoot(document.createElement('div')); - TestUtils.act(() => { + ReactTestUtils.act(() => { root.render( @@ -456,7 +469,7 @@ describe('updaters', () => { expect(allSchedulerTypes).toHaveLength(1); // Render a partially update, but don't finish. - TestUtils.act(() => { + ReactTestUtils.act(() => { triggerLowPriorityUpdate(); expect(Scheduler).toFlushAndYieldThrough(['LowPriorityUpdater 1']); expect(allSchedulerTypes).toHaveLength(1); @@ -487,4 +500,85 @@ describe('updaters', () => { // Verify no outstanding flushes Scheduler.flushAll(); }); + + it('should not lose track of updaters if work yields before finishing', () => { + loadModules({useDOMRenderer: false}); + + const {HostRoot} = require('shared/ReactWorkTags'); + + const Yield = ({renderTime}) => { + Scheduler.advanceTime(renderTime); + Scheduler.yieldValue('Yield:' + renderTime); + return null; + }; + + let first; + class FirstComponent extends React.Component { + state = {renderTime: 1}; + render() { + first = this; + Scheduler.advanceTime(this.state.renderTime); + Scheduler.yieldValue('FirstComponent:' + this.state.renderTime); + return ; + } + } + let second; + class SecondComponent extends React.Component { + state = {renderTime: 2}; + render() { + second = this; + Scheduler.advanceTime(this.state.renderTime); + Scheduler.yieldValue('SecondComponent:' + this.state.renderTime); + return ; + } + } + + Scheduler.advanceTime(5); // 0 -> 5 + + const renderer = ReactTestRenderer.create( + + + + , + {unstable_isConcurrent: true}, + ); + + // Render everything initially. + expect(Scheduler).toFlushAndYield([ + 'FirstComponent:1', + 'Yield:4', + 'SecondComponent:2', + 'Yield:7', + 'onCommitRoot', + ]); + expect(allSchedulerTags).toHaveLength(1); + expect(allSchedulerTags[0]).toHaveLength(1); + expect(allSchedulerTags[0]).toContain(HostRoot); + + // Render a partially update, but don't finish. + first.setState({renderTime: 10}); + expect(Scheduler).toFlushAndYieldThrough(['FirstComponent:10']); + expect(allSchedulerTypes).toHaveLength(1); + + // Interrupt with higher priority work. + renderer.unstable_flushSync(() => second.setState({renderTime: 30})); + expect(Scheduler).toHaveYielded([ + 'SecondComponent:30', + 'Yield:7', + 'onCommitRoot', + ]); + expect(allSchedulerTypes).toHaveLength(2); + expect(allSchedulerTypes[1]).toHaveLength(1); + expect(allSchedulerTypes[1]).toContain(SecondComponent); + + // Resume the original low priority update. + expect(Scheduler).toFlushAndYield([ + 'FirstComponent:10', + 'Yield:4', + 'onCommitRoot', + ]); + expect(allSchedulerTypes).toHaveLength(3); + expect(allSchedulerTypes[2]).toHaveLength(1); + expect(allSchedulerTypes[2]).toContain(FirstComponent); + }); });