Make root.unmount() synchronous (facebook#22444)

* Move flushSync warning to React DOM

When you call in `flushSync` from an effect, React fires a warning. I've
moved the implementation of this warning out of the reconciler and into
React DOM.

`flushSync` is a renderer API, not an isomorphic API, because it has
behavior that was designed specifically for the constraints of React
DOM. The equivalent API in a different renderer may not be the same.
For example, React Native has a different threading model than the
browser, so it might not make sense to expose a `flushSync` API to the
JavaScript thread.

* Make root.unmount() synchronous

When you unmount a root, the internal state that React stores on the
DOM node is immediately cleared. So, we should also synchronously
delete the React tree. You should be able to create a new root using
the same container.

Author: acdlite

packages/react-devtools-shared/src/__tests__/storeStressTestConcurrent-test.js

@@ -246,9 +246,9 @@ describe('StoreStressConcurrent', () => {
     // 1. Capture the expected render result.
     const snapshots = [];
     let container = document.createElement('div');
-    // $FlowFixMe
-    let root = ReactDOM.createRoot(container);
     for (let i = 0; i < steps.length; i++) {
+      // $FlowFixMe
+      const root = ReactDOM.createRoot(container);
       act(() => root.render(<Root>{steps[i]}</Root>));
       // We snapshot each step once so it doesn't regress.
       snapshots.push(print(store));
@@ -320,7 +320,7 @@ describe('StoreStressConcurrent', () => {
       for (let j = 0; j < steps.length; j++) {
         container = document.createElement('div');
         // $FlowFixMe
-        root = ReactDOM.createRoot(container);
+        const root = ReactDOM.createRoot(container);
         act(() => root.render(<Root>{steps[i]}</Root>));
         expect(print(store)).toMatch(snapshots[i]);
         act(() => root.render(<Root>{steps[j]}</Root>));
@@ -337,7 +337,7 @@ describe('StoreStressConcurrent', () => {
       for (let j = 0; j < steps.length; j++) {
         container = document.createElement('div');
         // $FlowFixMe
-        root = ReactDOM.createRoot(container);
+        const root = ReactDOM.createRoot(container);
         act(() =>
           root.render(
             <Root>
@@ -408,9 +408,9 @@ describe('StoreStressConcurrent', () => {
     // This is the only step where we use Jest snapshots.
     const snapshots = [];
     let container = document.createElement('div');
-    // $FlowFixMe
-    let root = ReactDOM.createRoot(container);
     for (let i = 0; i < steps.length; i++) {
+      // $FlowFixMe
+      const root = ReactDOM.createRoot(container);
       act(() =>
         root.render(
           <Root>
@@ -512,6 +512,8 @@ describe('StoreStressConcurrent', () => {
 
     // 2. Verify check Suspense can render same steps as initial fallback content.
     for (let i = 0; i < steps.length; i++) {
+      // $FlowFixMe
+      const root = ReactDOM.createRoot(container);
       act(() =>
         root.render(
           <Root>
@@ -536,7 +538,7 @@ describe('StoreStressConcurrent', () => {
         // Always start with a fresh container and steps[i].
         container = document.createElement('div');
         // $FlowFixMe
-        root = ReactDOM.createRoot(container);
+        const root = ReactDOM.createRoot(container);
         act(() =>
           root.render(
             <Root>
@@ -582,7 +584,7 @@ describe('StoreStressConcurrent', () => {
         // Always start with a fresh container and steps[i].
         container = document.createElement('div');
         // $FlowFixMe
-        root = ReactDOM.createRoot(container);
+        const root = ReactDOM.createRoot(container);
         act(() =>
           root.render(
             <Root>
@@ -640,7 +642,7 @@ describe('StoreStressConcurrent', () => {
         // Always start with a fresh container and steps[i].
         container = document.createElement('div');
         // $FlowFixMe
-        root = ReactDOM.createRoot(container);
+        const root = ReactDOM.createRoot(container);
         act(() =>
           root.render(
             <Root>
@@ -690,7 +692,7 @@ describe('StoreStressConcurrent', () => {
         // Always start with a fresh container and steps[i].
         container = document.createElement('div');
         // $FlowFixMe
-        root = ReactDOM.createRoot(container);
+        const root = ReactDOM.createRoot(container);
         act(() =>
           root.render(
             <Root>
@@ -744,7 +746,7 @@ describe('StoreStressConcurrent', () => {
         // Always start with a fresh container and steps[i].
         container = document.createElement('div');
         // $FlowFixMe
-        root = ReactDOM.createRoot(container);
+        const root = ReactDOM.createRoot(container);
         act(() =>
           root.render(
             <Root>
@@ -897,9 +899,9 @@ describe('StoreStressConcurrent', () => {
     // This is the only step where we use Jest snapshots.
     const snapshots = [];
     let container = document.createElement('div');
-    // $FlowFixMe
-    let root = ReactDOM.createRoot(container);
     for (let i = 0; i < steps.length; i++) {
+      // $FlowFixMe
+      const root = ReactDOM.createRoot(container);
       act(() =>
         root.render(
           <Root>
@@ -922,6 +924,8 @@ describe('StoreStressConcurrent', () => {
     // which is different from the snapshots above. So we take more snapshots.
     const fallbackSnapshots = [];
     for (let i = 0; i < steps.length; i++) {
+      // $FlowFixMe
+      const root = ReactDOM.createRoot(container);
       act(() =>
         root.render(
           <Root>
@@ -1055,7 +1059,7 @@ describe('StoreStressConcurrent', () => {
         // Always start with a fresh container and steps[i].
         container = document.createElement('div');
         // $FlowFixMe
-        root = ReactDOM.createRoot(container);
+        const root = ReactDOM.createRoot(container);
         act(() =>
           root.render(
             <Root>
@@ -1107,7 +1111,7 @@ describe('StoreStressConcurrent', () => {
         // Always start with a fresh container and steps[i].
         container = document.createElement('div');
         // $FlowFixMe
-        root = ReactDOM.createRoot(container);
+        const root = ReactDOM.createRoot(container);
         act(() =>
           root.render(
             <Root>
@@ -1174,7 +1178,7 @@ describe('StoreStressConcurrent', () => {
         // Always start with a fresh container and steps[i].
         container = document.createElement('div');
         // $FlowFixMe
-        root = ReactDOM.createRoot(container);
+        const root = ReactDOM.createRoot(container);
         act(() =>
           root.render(
             <Root>
@@ -1226,7 +1230,7 @@ describe('StoreStressConcurrent', () => {
         // Always start with a fresh container and steps[i].
         container = document.createElement('div');
         // $FlowFixMe
-        root = ReactDOM.createRoot(container);
+        const root = ReactDOM.createRoot(container);
         act(() =>
           root.render(
             <Root>
@@ -1278,7 +1282,7 @@ describe('StoreStressConcurrent', () => {
         // Always start with a fresh container and steps[i].
         container = document.createElement('div');
         // $FlowFixMe
-        root = ReactDOM.createRoot(container);
+        const root = ReactDOM.createRoot(container);
         act(() =>
           root.render(
             <Root>

packages/react-dom/src/__tests__/ReactDOMRoot-test.js

@@ -14,6 +14,7 @@ let ReactDOM = require('react-dom');
 let ReactDOMServer = require('react-dom/server');
 let Scheduler = require('scheduler');
 let act;
+let useEffect;
 
 describe('ReactDOMRoot', () => {
   let container;
@@ -26,6 +27,7 @@ describe('ReactDOMRoot', () => {
     ReactDOMServer = require('react-dom/server');
     Scheduler = require('scheduler');
     act = require('jest-react').act;
+    useEffect = React.useEffect;
   });
 
   it('renders children', () => {
@@ -342,4 +344,62 @@ describe('ReactDOMRoot', () => {
     });
     expect(container.textContent).toEqual('b');
   });
+
+  it('unmount is synchronous', async () => {
+    const root = ReactDOM.createRoot(container);
+    await act(async () => {
+      root.render('Hi');
+    });
+    expect(container.textContent).toEqual('Hi');
+
+    await act(async () => {
+      root.unmount();
+      // Should have already unmounted
+      expect(container.textContent).toEqual('');
+    });
+  });
+
+  it('throws if an unmounted root is updated', async () => {
+    const root = ReactDOM.createRoot(container);
+    await act(async () => {
+      root.render('Hi');
+    });
+    expect(container.textContent).toEqual('Hi');
+
+    root.unmount();
+
+    expect(() => root.render("I'm back")).toThrow(
+      'Cannot update an unmounted root.',
+    );
+  });
+
+  it('warns if root is unmounted inside an effect', async () => {
+    const container1 = document.createElement('div');
+    const root1 = ReactDOM.createRoot(container1);
+    const container2 = document.createElement('div');
+    const root2 = ReactDOM.createRoot(container2);
+
+    function App({step}) {
+      useEffect(() => {
+        if (step === 2) {
+          root2.unmount();
+        }
+      }, [step]);
+      return 'Hi';
+    }
+
+    await act(async () => {
+      root1.render(<App step={1} />);
+    });
+    expect(container1.textContent).toEqual('Hi');
+
+    expect(() => {
+      ReactDOM.flushSync(() => {
+        root1.render(<App step={2} />);
+      });
+    }).toErrorDev(
+      'Attempted to synchronously unmount a root while React was ' +
+        'already rendering.',
+    );
+  });
 });

packages/react-dom/src/client/ReactDOM.js

@@ -23,8 +23,8 @@ import {createEventHandle} from './ReactDOMEventHandle';
 import {
   batchedUpdates,
   discreteUpdates,
-  flushSync,
-  flushSyncWithoutWarningIfAlreadyRendering,
+  flushSync as flushSyncWithoutWarningIfAlreadyRendering,
+  isAlreadyRendering,
   flushControlled,
   injectIntoDevTools,
   attemptSynchronousHydration,
@@ -163,6 +163,25 @@ const Internals = {
   ],
 };
 
+// Overload the definition to the two valid signatures.
+// Warning, this opts-out of checking the function body.
+declare function flushSync<R>(fn: () => R): R;
+// eslint-disable-next-line no-redeclare
+declare function flushSync(): void;
+// eslint-disable-next-line no-redeclare
+function flushSync(fn) {
+  if (__DEV__) {
+    if (isAlreadyRendering()) {
+      console.error(
+        'flushSync was called from inside a lifecycle method. React cannot ' +
+          'flush when React is already rendering. Consider moving this call to ' +
+          'a scheduler task or micro task.',
+      );
+    }
+  }
+  return flushSyncWithoutWarningIfAlreadyRendering(fn);
+}
+
 export {
   createPortal,
   batchedUpdates as unstable_batchedUpdates,

packages/react-dom/src/client/ReactDOMLegacy.js

@@ -29,7 +29,7 @@ import {
   createContainer,
   findHostInstanceWithNoPortals,
   updateContainer,
-  flushSyncWithoutWarningIfAlreadyRendering,
+  flushSync,
   getPublicRootInstance,
   findHostInstance,
   findHostInstanceWithWarning,
@@ -174,7 +174,7 @@ function legacyRenderSubtreeIntoContainer(
       };
     }
     // Initial mount should not be batched.
-    flushSyncWithoutWarningIfAlreadyRendering(() => {
+    flushSync(() => {
       updateContainer(children, fiberRoot, parentComponent, callback);
     });
   } else {
@@ -357,7 +357,7 @@ export function unmountComponentAtNode(container: Container) {
     }
 
     // Unmount should not be batched.
-    flushSyncWithoutWarningIfAlreadyRendering(() => {
+    flushSync(() => {
       legacyRenderSubtreeIntoContainer(null, null, container, false, () => {
         // $FlowFixMe This should probably use `delete container._reactRootContainer`
         container._reactRootContainer = null;

packages/react-dom/src/client/ReactDOMRoot.js

@@ -14,7 +14,7 @@ import type {FiberRoot} from 'react-reconciler/src/ReactInternalTypes';
 export type RootType = {
   render(children: ReactNodeList): void,
   unmount(): void,
-  _internalRoot: FiberRoot,
+  _internalRoot: FiberRoot | null,
   ...
 };
 
@@ -62,17 +62,23 @@ import {
   updateContainer,
   findHostInstanceWithNoPortals,
   registerMutableSourceForHydration,
+  flushSync,
+  isAlreadyRendering,
 } from 'react-reconciler/src/ReactFiberReconciler';
 import invariant from 'shared/invariant';
 import {ConcurrentRoot} from 'react-reconciler/src/ReactRootTags';
 import {allowConcurrentByDefault} from 'shared/ReactFeatureFlags';
 
-function ReactDOMRoot(internalRoot) {
+function ReactDOMRoot(internalRoot: FiberRoot) {
   this._internalRoot = internalRoot;
 }
 
 ReactDOMRoot.prototype.render = function(children: ReactNodeList): void {
   const root = this._internalRoot;
+  if (root === null) {
+    invariant(false, 'Cannot update an unmounted root.');
+  }
+
   if (__DEV__) {
     if (typeof arguments[1] === 'function') {
       console.error(
@@ -109,10 +115,23 @@ ReactDOMRoot.prototype.unmount = function(): void {
     }
   }
   const root = this._internalRoot;
-  const container = root.containerInfo;
-  updateContainer(null, root, null, () => {
+  if (root !== null) {
+    this._internalRoot = null;
+    const container = root.containerInfo;
+    if (__DEV__) {
+      if (isAlreadyRendering()) {
+        console.error(
+          'Attempted to synchronously unmount a root while React was already ' +
+            'rendering. React cannot finish unmounting the root until the ' +
+            'current render has completed, which may lead to a race condition.',
+        );
+      }
+    }
+    flushSync(() => {
+      updateContainer(null, root, null, null);
+    });
     unmarkContainerAsRoot(container);
-  });
+  }
 };
 
 export function createRoot(

packages/react-noop-renderer/src/createReactNoop.js

@@ -921,6 +921,19 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
     return children;
   }
 
+  function flushSync<R>(fn: () => R): R {
+    if (__DEV__) {
+      if (NoopRenderer.isAlreadyRendering()) {
+        console.error(
+          'flushSync was called from inside a lifecycle method. React cannot ' +
+            'flush when React is already rendering. Consider moving this call to ' +
+            'a scheduler task or micro task.',
+        );
+      }
+    }
+    return NoopRenderer.flushSync(fn);
+  }
+
   let idCounter = 0;
 
   const ReactNoop = {
@@ -1136,7 +1149,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
       }
     },
 
-    flushSync: NoopRenderer.flushSync,
+    flushSync,
     flushPassiveEffects: NoopRenderer.flushPassiveEffects,
 
     // Logs the current state of the tree.