Don't "schedule" discrete work if we're scheduling sync work

Author: sebmarkbage

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 (
+        <button onClick={hideOnClick} ref={ref}>
+          {value}
+        </button>
+      );
+    }
+
+    function NewApp() {
+      return <button ref={ref}>new</button>;
+    }
+
+    const oldRoot = ReactDOM.createRoot(container);
+    act(() => {
+      oldRoot.render(<OldApp />);
+    });
+
+    // 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(<NewApp />);
+    });
+    ref.current.click();
+
+    expect(container.textContent).toBe('new');
+  });
 });

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);

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);

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<A, R>(fn: A => R, a: A): R {

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<A, R>(fn: A => R, a: A): R {