Root API should clear non-empty roots before mounting (#18730)

* Root API should clear non-empty roots before mounting Legacy render-into-subtree API removes children from a container before rendering into it. The root API did not do this previously, but just left the children around in the document. This commit adds a new FiberRoot flag to clear a container's contents before mounting. This is done during the commit phase, to avoid multiple, observable mutations.
facebook · Apr 28, 2020 · ea2af87 · ea2af87
1 parent d2ef120
commit ea2af87
Show file tree

Hide file tree

Showing 13 changed files with 110 additions and 7 deletions.
diff --git a/packages/react-art/src/ReactARTHostConfig.js b/packages/react-art/src/ReactARTHostConfig.js
@@ -427,6 +427,10 @@ export function unhideTextInstance(textInstance, text): void {
   // Noop
 }
 
+export function clearContainer(container) {
+  // TODO Implement this
+}
+
 export function DEPRECATED_mountResponderInstance(
   responder: ReactEventResponder<any, any>,
   responderInstance: ReactEventResponderInstance<any, any>,

diff --git a/packages/react-dom/src/__tests__/ReactDOMRoot-test.js b/packages/react-dom/src/__tests__/ReactDOMRoot-test.js
@@ -113,7 +113,28 @@ describe('ReactDOMRoot', () => {
     expect(() => Scheduler.unstable_flushAll()).toErrorDev('Extra attributes');
   });
 
-  it('does not clear existing children', async () => {
+  it('clears existing children with legacy API', async () => {
+    container.innerHTML = '<div>a</div><div>b</div>';
+    ReactDOM.render(
+      <div>
+        <span>c</span>
+        <span>d</span>
+      </div>,
+      container,
+    );
+    expect(container.textContent).toEqual('cd');
+    ReactDOM.render(
+      <div>
+        <span>d</span>
+        <span>c</span>
+      </div>,
+      container,
+    );
+    Scheduler.unstable_flushAll();
+    expect(container.textContent).toEqual('dc');
+  });
+
+  it('clears existing children', async () => {
     container.innerHTML = '<div>a</div><div>b</div>';
     const root = ReactDOM.createRoot(container);
     root.render(
@@ -123,15 +144,15 @@ describe('ReactDOMRoot', () => {
       </div>,
     );
     Scheduler.unstable_flushAll();
-    expect(container.textContent).toEqual('abcd');
+    expect(container.textContent).toEqual('cd');
     root.render(
       <div>
         <span>d</span>
         <span>c</span>
       </div>,
     );
     Scheduler.unstable_flushAll();
-    expect(container.textContent).toEqual('abdc');
+    expect(container.textContent).toEqual('dc');
   });
 
   it('throws a good message on invalid containers', () => {
@@ -220,7 +241,14 @@ describe('ReactDOMRoot', () => {
     let unmounted = false;
     expect(() => {
       unmounted = ReactDOM.unmountComponentAtNode(container);
-    }).toErrorDev('Did you mean to call root.unmount()?', {withoutStack: true});
+    }).toErrorDev(
+      [
+        'Did you mean to call root.unmount()?',
+        // This is more of a symptom but restructuring the code to avoid it isn't worth it:
+        "The node you're attempting to unmount was rendered by React and is not a top-level container.",
+      ],
+      {withoutStack: true},
+    );
     expect(unmounted).toBe(false);
     Scheduler.unstable_flushAll();
     expect(container.textContent).toEqual('Hi');

diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js
@@ -634,6 +634,17 @@ export function unhideTextInstance(
   textInstance.nodeValue = text;
 }
 
+export function clearContainer(container: Container): void {
+  if (container.nodeType === ELEMENT_NODE) {
+    ((container: any): Element).textContent = '';
+  } else if (container.nodeType === DOCUMENT_NODE) {
+    const body = ((container: any): Document).body;
+    if (body != null) {
+      body.textContent = '';
+    }
+  }
+}
+
 // -------------------
 //     Hydration
 // -------------------

diff --git a/packages/react-native-renderer/src/ReactNativeHostConfig.js b/packages/react-native-renderer/src/ReactNativeHostConfig.js
@@ -482,6 +482,11 @@ export function unhideInstance(instance: Instance, props: Props): void {
   );
 }
 
+export function clearContainer(container: Container): void {
+  // TODO Implement this for React Native
+  // UIManager does not expose a "remove all" type method.
+}
+
 export function unhideTextInstance(
   textInstance: TextInstance,
   text: string,

diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js
@@ -155,6 +155,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
     insertInContainerOrInstanceBefore(parentInstance, child, beforeChild);
   }
 
+  function clearContainer(container: Container): void {
+    container.children.splice(0);
+  }
+
   function removeChildFromContainerOrInstance(
     parentInstance: Container | Instance,
     child: Instance | TextInstance,
@@ -502,6 +506,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
         insertInContainerBefore,
         removeChild,
         removeChildFromContainer,
+        clearContainer,
 
         hideInstance(instance: Instance): void {
           instance.hidden = true;
@@ -531,6 +536,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
         supportsPersistence: true,
 
         cloneInstance,
+        clearContainer,
 
         createContainerChildSet(
           container: Container,

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -113,6 +113,7 @@ import {
   commitHydratedContainer,
   commitHydratedSuspenseInstance,
   beforeRemoveInstance,
+  clearContainer,
 } from './ReactFiberHostConfig';
 import {
   captureCommitPhaseError,
@@ -295,7 +296,15 @@ function commitBeforeMutationLifeCycles(
       }
       return;
     }
-    case HostRoot:
+    case HostRoot: {
+      if (supportsMutation) {
+        if (finishedWork.effectTag & Snapshot) {
+          const root = finishedWork.stateNode;
+          clearContainer(root.containerInfo);
+        }
+      }
+      return;
+    }
     case HostComponent:
     case HostText:
     case HostPortal:

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
@@ -111,6 +111,7 @@ import {
   commitHydratedContainer,
   commitHydratedSuspenseInstance,
   beforeRemoveInstance,
+  clearContainer,
 } from './ReactFiberHostConfig';
 import {
   captureCommitPhaseError,
@@ -293,7 +294,15 @@ function commitBeforeMutationLifeCycles(
       }
       return;
     }
-    case HostRoot:
+    case HostRoot: {
+      if (supportsMutation) {
+        if (finishedWork.effectTag & Snapshot) {
+          const root = finishedWork.stateNode;
+          clearContainer(root.containerInfo);
+        }
+      }
+      return;
+    }
     case HostComponent:
     case HostText:
     case HostPortal:

diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.new.js b/packages/react-reconciler/src/ReactFiberCompleteWork.new.js
@@ -58,7 +58,13 @@ import {
   OffscreenComponent,
 } from './ReactWorkTags';
 import {NoMode, BlockingMode} from './ReactTypeOfMode';
-import {Ref, Update, NoEffect, DidCapture} from './ReactSideEffectTags';
+import {
+  Ref,
+  Update,
+  NoEffect,
+  DidCapture,
+  Snapshot,
+} from './ReactSideEffectTags';
 import invariant from 'shared/invariant';
 
 import {
@@ -675,6 +681,14 @@ function completeWork(
           // If we hydrated, then we'll need to schedule an update for
           // the commit side-effects on the root.
           markUpdate(workInProgress);
+        } else if (!fiberRoot.hydrate) {
+          // Schedule an effect to clear this container at the start of the next commit.
+          // 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 is required for hydration in legacy mode with no fallback.
+          workInProgress.effectTag |= Snapshot;
         }
       }
       updateHostContainer(workInProgress);

diff --git a/packages/react-reconciler/src/ReactFiberCompleteWork.old.js b/packages/react-reconciler/src/ReactFiberCompleteWork.old.js
@@ -61,6 +61,7 @@ import {
   NoEffect,
   DidCapture,
   Deletion,
+  Snapshot,
 } from './ReactSideEffectTags';
 import invariant from 'shared/invariant';
 
@@ -678,6 +679,14 @@ function completeWork(
           // If we hydrated, then we'll need to schedule an update for
           // the commit side-effects on the root.
           markUpdate(workInProgress);
+        } else if (!fiberRoot.hydrate) {
+          // Schedule an effect to clear this container at the start of the next commit.
+          // 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 is required for hydration in legacy mode with no fallback.
+          workInProgress.effectTag |= Snapshot;
         }
       }
       updateHostContainer(workInProgress);

diff --git a/packages/react-reconciler/src/ReactFiberHostConfigWithNoMutation.js b/packages/react-reconciler/src/ReactFiberHostConfigWithNoMutation.js
@@ -37,3 +37,4 @@ export const hideInstance = shim;
 export const hideTextInstance = shim;
 export const unhideInstance = shim;
 export const unhideTextInstance = shim;
+export const clearContainer = shim;
diff --git a/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js b/packages/react-reconciler/src/__tests__/ReactFiberHostContext-test.internal.js
@@ -53,6 +53,7 @@ describe('ReactFiberHostContext', () => {
       appendChildToContainer: function() {
         return null;
       },
+      clearContainer: function() {},
       supportsMutation: true,
     });
 
@@ -107,6 +108,7 @@ describe('ReactFiberHostContext', () => {
       appendChildToContainer: function() {
         return null;
       },
+      clearContainer: function() {},
       supportsMutation: true,
     });
 

diff --git a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js
@@ -107,6 +107,7 @@ export const updateFundamentalComponent =
   $$$hostConfig.updateFundamentalComponent;
 export const unmountFundamentalComponent =
   $$$hostConfig.unmountFundamentalComponent;
+export const clearContainer = $$$hostConfig.clearContainer;
 
 // -------------------
 //     Persistence

diff --git a/packages/react-test-renderer/src/ReactTestHostConfig.js b/packages/react-test-renderer/src/ReactTestHostConfig.js
@@ -126,6 +126,10 @@ export function removeChild(
   parentInstance.children.splice(index, 1);
 }
 
+export function clearContainer(container: Container): void {
+  container.children.splice(0);
+}
+
 export function getRootHostContext(
   rootContainerInstance: Container,
 ): HostContext {