From 486b52ef3e62dbd63616cbf2821c074efb51fd56 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Thu, 30 Apr 2020 21:27:56 -0700 Subject: [PATCH] Don't "schedule" discrete work if we're scheduling sync work --- .../src/__tests__/ReactDOMFiberAsync-test.js | 46 +++++++++++++++++ .../src/ReactFiberCompleteWork.new.js | 17 +------ .../src/ReactFiberCompleteWork.old.js | 17 +------ .../src/ReactFiberWorkLoop.new.js | 49 ++++++++++--------- .../src/ReactFiberWorkLoop.old.js | 46 +++++++++-------- 5 files changed, 98 insertions(+), 77 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js index 77dd7e53355da..c98fdabcbcf26 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js @@ -13,6 +13,7 @@ let React; let ReactDOM; let Scheduler; +let act; const setUntrackedInputValue = Object.getOwnPropertyDescriptor( HTMLInputElement.prototype, @@ -27,6 +28,7 @@ describe('ReactDOMFiberAsync', () => { container = document.createElement('div'); React = require('react'); ReactDOM = require('react-dom'); + act = require('react-dom/test-utils').act; Scheduler = require('scheduler'); document.body.appendChild(container); @@ -635,4 +637,48 @@ describe('ReactDOMFiberAsync', () => { expect(container.textContent).toEqual('ABC'); }); }); + + // @gate experimental + it('unmounted roots should never clear newer root content from a container', () => { + const ref = React.createRef(); + + function OldApp() { + const [value, setValue] = React.useState('old'); + function hideOnClick() { + // Schedule a discrete update. + setValue('update'); + // Synchronously unmount this root. + ReactDOM.flushSync(() => oldRoot.unmount()); + } + return ( + + ); + } + + function NewApp() { + return ; + } + + const oldRoot = ReactDOM.createRoot(container); + act(() => { + oldRoot.render(); + }); + + // Invoke discrete event. + ref.current.click(); + + // The root should now be unmounted. + expect(container.textContent).toBe(''); + + // We can now render a new one. + const newRoot = ReactDOM.createRoot(container); + ReactDOM.flushSync(() => { + newRoot.render(); + }); + ref.current.click(); + + expect(container.textContent).toBe('new'); + }); }); diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js index 620279aced2e8..1dc93a73bc703 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js @@ -686,22 +686,7 @@ function completeWork( // This handles the case of React rendering into a container with previous children. // It's also safe to do for updates too, because current.child would only be null // if the previous render was null (so the the container would already be empty). - // - // The additional root.hydrate check above is required for hydration in legacy mode with no fallback. - // - // The root container check below also avoids a potential legacy mode problem - // where unmounting from a container then rendering into it again - // can sometimes cause the container to be cleared after the new render. - const containerInfo = fiberRoot.containerInfo; - const legacyRootContainer = - containerInfo == null ? null : containerInfo._reactRootContainer; - if ( - legacyRootContainer == null || - legacyRootContainer._internalRoot == null || - legacyRootContainer._internalRoot === fiberRoot - ) { - workInProgress.effectTag |= Snapshot; - } + workInProgress.effectTag |= Snapshot; } } updateHostContainer(workInProgress); diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js index 818d08ca469f4..82ec4d95f75e3 100644 --- a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js @@ -684,22 +684,7 @@ function completeWork( // This handles the case of React rendering into a container with previous children. // It's also safe to do for updates too, because current.child would only be null // if the previous render was null (so the the container would already be empty). - // - // The additional root.hydrate check above is required for hydration in legacy mode with no fallback. - // - // The root container check below also avoids a potential legacy mode problem - // where unmounting from a container then rendering into it again - // can sometimes cause the container to be cleared after the new render. - const containerInfo = fiberRoot.containerInfo; - const legacyRootContainer = - containerInfo == null ? null : containerInfo._reactRootContainer; - if ( - legacyRootContainer == null || - legacyRootContainer._internalRoot == null || - legacyRootContainer._internalRoot === fiberRoot - ) { - workInProgress.effectTag |= Snapshot; - } + workInProgress.effectTag |= Snapshot; } } updateHostContainer(workInProgress); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index a6c71a1560cdf..03a2acb0ae851 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -474,30 +474,31 @@ export function scheduleUpdateOnFiber( } } } else { - ensureRootIsScheduled(root); - schedulePendingInteractions(root, expirationTime); - } - - if ( - (executionContext & DiscreteEventContext) !== NoContext && - // Only updates at user-blocking priority or greater are considered - // discrete, even inside a discrete event. - (priorityLevel === UserBlockingSchedulerPriority || - priorityLevel === ImmediateSchedulerPriority) - ) { - // This is the result of a discrete event. Track the lowest priority - // discrete update per root so we can flush them early, if needed. - if (rootsWithPendingDiscreteUpdates === null) { - rootsWithPendingDiscreteUpdates = new Map([[root, expirationTime]]); - } else { - const lastDiscreteTime = rootsWithPendingDiscreteUpdates.get(root); - if ( - lastDiscreteTime === undefined || - !isSameOrHigherPriority(expirationTime, lastDiscreteTime) - ) { - rootsWithPendingDiscreteUpdates.set(root, expirationTime); + // Schedule a discrete update but only if it's not Sync. + if ( + (executionContext & DiscreteEventContext) !== NoContext && + // Only updates at user-blocking priority or greater are considered + // discrete, even inside a discrete event. + (priorityLevel === UserBlockingSchedulerPriority || + priorityLevel === ImmediateSchedulerPriority) + ) { + // This is the result of a discrete event. Track the lowest priority + // discrete update per root so we can flush them early, if needed. + if (rootsWithPendingDiscreteUpdates === null) { + rootsWithPendingDiscreteUpdates = new Map([[root, expirationTime]]); + } else { + const lastDiscreteTime = rootsWithPendingDiscreteUpdates.get(root); + if ( + lastDiscreteTime === undefined || + !isSameOrHigherPriority(expirationTime, lastDiscreteTime) + ) { + rootsWithPendingDiscreteUpdates.set(root, expirationTime); + } } } + // Schedule other updates after in case the callback is sync. + ensureRootIsScheduled(root); + schedulePendingInteractions(root, expirationTime); } } @@ -1155,9 +1156,9 @@ function flushPendingDiscreteUpdates() { markRootExpiredAtTime(root, expirationTime); ensureRootIsScheduled(root); }); - // Now flush the immediate queue. - flushSyncCallbackQueue(); } + // Now flush the immediate queue. + flushSyncCallbackQueue(); } export function batchedUpdates(fn: A => R, a: A): R { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 5cfac77d88814..daadbbc570fd3 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -456,27 +456,31 @@ export function scheduleUpdateOnFiber( } } } else { - ensureRootIsScheduled(root); - schedulePendingInteractions(root, expirationTime); - } - - if ( - (executionContext & DiscreteEventContext) !== NoContext && - // Only updates at user-blocking priority or greater are considered - // discrete, even inside a discrete event. - (priorityLevel === UserBlockingPriority || - priorityLevel === ImmediatePriority) - ) { - // This is the result of a discrete event. Track the lowest priority - // discrete update per root so we can flush them early, if needed. - if (rootsWithPendingDiscreteUpdates === null) { - rootsWithPendingDiscreteUpdates = new Map([[root, expirationTime]]); - } else { - const lastDiscreteTime = rootsWithPendingDiscreteUpdates.get(root); - if (lastDiscreteTime === undefined || lastDiscreteTime > expirationTime) { - rootsWithPendingDiscreteUpdates.set(root, expirationTime); + // Schedule a discrete update but only if it's not Sync. + if ( + (executionContext & DiscreteEventContext) !== NoContext && + // Only updates at user-blocking priority or greater are considered + // discrete, even inside a discrete event. + (priorityLevel === UserBlockingPriority || + priorityLevel === ImmediatePriority) + ) { + // This is the result of a discrete event. Track the lowest priority + // discrete update per root so we can flush them early, if needed. + if (rootsWithPendingDiscreteUpdates === null) { + rootsWithPendingDiscreteUpdates = new Map([[root, expirationTime]]); + } else { + const lastDiscreteTime = rootsWithPendingDiscreteUpdates.get(root); + if ( + lastDiscreteTime === undefined || + lastDiscreteTime > expirationTime + ) { + rootsWithPendingDiscreteUpdates.set(root, expirationTime); + } } } + // Schedule other updates after in case the callback is sync. + ensureRootIsScheduled(root); + schedulePendingInteractions(root, expirationTime); } } @@ -1080,9 +1084,9 @@ function flushPendingDiscreteUpdates() { markRootExpiredAtTime(root, expirationTime); ensureRootIsScheduled(root); }); - // Now flush the immediate queue. - flushSyncCallbackQueue(); } + // Now flush the immediate queue. + flushSyncCallbackQueue(); } export function batchedUpdates(fn: A => R, a: A): R {