Replaced new root attribute with ClearContainer effect

facebook · Apr 25, 2020 · 88d6d34 · 88d6d34
1 parent fad4529
commit 88d6d34
Show file tree

Hide file tree

Showing 17 changed files with 94 additions and 52 deletions.
diff --git a/packages/react-art/src/ReactARTHostConfig.js b/packages/react-art/src/ReactARTHostConfig.js
@@ -427,6 +427,14 @@ export function unhideTextInstance(textInstance, text): void {
   // Noop
 }
 
+export function clearContainer(container: Container): void {
+  // TODO This doesn't work for anything other than SVG.
+  // Is that okay?
+  while (container.lastChild != null) {
+    container.lastChild.eject();
+  }
+}
+
 export function DEPRECATED_mountResponderInstance(
   responder: ReactEventResponder<any, any>,
   responderInstance: ReactEventResponderInstance<any, any>,

diff --git a/packages/react-dom/src/client/ReactDOMRoot.js b/packages/react-dom/src/client/ReactDOMRoot.js
@@ -123,20 +123,12 @@ function createRootImpl(
     (options != null && options.hydrationOptions) || null;
   const root = createContainer(container, tag, hydrate, hydrationCallbacks);
   markContainerAsRoot(root.current, container);
-  if (hydrate) {
-    if (tag !== LegacyRoot) {
-      const doc =
-        container.nodeType === DOCUMENT_NODE
-          ? container
-          : container.ownerDocument;
-      eagerlyTrapReplayableEvents(container, doc);
-    }
-  } else if (container.lastChild != null) {
-    // If the container has children already and we aren't hydrating-
-    // schedule them to be cleared before we mount new, React-managed children.
-    // This mimics legacy render into subtree behavior in a way that is safe for concurrent mode.
-    // (It doesn't result in multiple obsevable mutations.)
-    root.clearContainerBeforeMount = true;
+  if (hydrate && tag !== LegacyRoot) {
+    const doc =
+      container.nodeType === DOCUMENT_NODE
+        ? container
+        : container.ownerDocument;
+    eagerlyTrapReplayableEvents(container, doc);
   }
   return root;
 }

diff --git a/packages/react-dom/src/client/__tests__/trustedTypes-test.internal.js b/packages/react-dom/src/client/__tests__/trustedTypes-test.internal.js
@@ -74,7 +74,8 @@ describe('when Trusted Types are available in global object', () => {
         container,
       );
       expect(container.innerHTML).toBe('<div><b>Hi</b></div>');
-      expect(innerHTMLCalls.length).toBe(1);
+      // Second call to innerHTML is the ClearContainer check.
+      expect(innerHTMLCalls.length).toBe(2);
       // Ensure it didn't get stringified when passed to a DOM sink:
       expect(innerHTMLCalls[0]).toBe(ttObject1);
 

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
@@ -68,6 +68,7 @@ import {
   Snapshot,
   Update,
   Passive,
+  ClearContainer,
 } from './ReactSideEffectTags';
 import getComponentName from 'shared/getComponentName';
 import invariant from 'shared/invariant';
@@ -111,6 +112,7 @@ import {
   commitHydratedContainer,
   commitHydratedSuspenseInstance,
   beforeRemoveInstance,
+  clearContainer,
 } from './ReactFiberHostConfig';
 import {
   captureCommitPhaseError,
@@ -293,7 +295,15 @@ function commitBeforeMutationLifeCycles(
       }
       return;
     }
-    case HostRoot:
+    case HostRoot: {
+      if (supportsMutation) {
+        if (finishedWork.effectTag & ClearContainer) {
+          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
@@ -68,6 +68,7 @@ import {
   Snapshot,
   Update,
   Passive,
+  ClearContainer,
 } from './ReactSideEffectTags';
 import getComponentName from 'shared/getComponentName';
 import invariant from 'shared/invariant';
@@ -111,6 +112,7 @@ import {
   commitHydratedContainer,
   commitHydratedSuspenseInstance,
   beforeRemoveInstance,
+  clearContainer,
 } from './ReactFiberHostConfig';
 import {
   captureCommitPhaseError,
@@ -293,7 +295,15 @@ function commitBeforeMutationLifeCycles(
       }
       return;
     }
-    case HostRoot:
+    case HostRoot: {
+      if (supportsMutation) {
+        if (finishedWork.effectTag & ClearContainer) {
+          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
@@ -61,6 +61,7 @@ import {
   NoEffect,
   DidCapture,
   Deletion,
+  ClearContainer,
 } from './ReactSideEffectTags';
 import invariant from 'shared/invariant';
 
@@ -678,6 +679,12 @@ function completeWork(
           // If we hydrated, then we'll need to schedule an update for
           // the commit side-effects on the root.
           markUpdate(workInProgress);
+        } else {
+          // 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).
+          workInProgress.effectTag |= ClearContainer;
         }
       }
       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,
+  ClearContainer,
 } from './ReactSideEffectTags';
 import invariant from 'shared/invariant';
 
@@ -678,6 +679,12 @@ function completeWork(
           // If we hydrated, then we'll need to schedule an update for
           // the commit side-effects on the root.
           markUpdate(workInProgress);
+        } else {
+          // 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).
+          workInProgress.effectTag |= ClearContainer;
         }
       }
       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/ReactFiberRoot.old.js b/packages/react-reconciler/src/ReactFiberRoot.old.js
@@ -45,7 +45,6 @@ function FiberRootNode(containerInfo, tag, hydrate) {
   this.lastPingedTime = NoWork;
   this.lastExpiredTime = NoWork;
   this.mutableSourceLastPendingUpdateTime = NoWork;
-  this.clearContainerBeforeMount = false;
 
   if (enableSchedulerTracing) {
     this.interactionThreadID = unstable_getThreadID();

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -112,6 +112,7 @@ import {
   HostEffectMask,
   Hydrating,
   HydratingAndUpdate,
+  ClearContainer,
 } from './ReactSideEffectTags';
 import {
   NoWork,
@@ -2063,7 +2064,7 @@ function commitBeforeMutationEffects() {
       beforeActiveInstanceBlur();
     }
     const effectTag = nextEffect.effectTag;
-    if ((effectTag & Snapshot) !== NoEffect) {
+    if ((effectTag & (Snapshot | ClearContainer)) !== NoEffect) {
       setCurrentDebugFiberInDEV(nextEffect);
 
       const current = nextEffect.alternate;

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -74,7 +74,6 @@ import {
   warnsIfNotActing,
   beforeActiveInstanceBlur,
   afterActiveInstanceBlur,
-  clearContainer,
 } from './ReactFiberHostConfig';
 
 import {
@@ -124,6 +123,7 @@ import {
   HostEffectMask,
   Hydrating,
   HydratingAndUpdate,
+  ClearContainer,
 } from './ReactSideEffectTags';
 import {
   NoWork,
@@ -1913,15 +1913,6 @@ function commitRootImpl(root, renderPriorityLevel) {
     firstEffect = finishedWork.firstEffect;
   }
 
-  if (root.clearContainerBeforeMount) {
-    // We are about to mount into a container that previous contained non-React elements.
-    // We should clear the previous contents before beginning.
-    // This mimics legacy render into subtree behavior in a way that is safe for concurrent mode.
-    // (It doesn't result in multiple obsevable mutations.)
-    root.clearContainerBeforeMount = false;
-    clearContainer(root.containerInfo);
-  }
-
   if (firstEffect !== null) {
     const prevExecutionContext = executionContext;
     executionContext |= CommitContext;
@@ -2177,7 +2168,7 @@ function commitBeforeMutationEffects() {
       beforeActiveInstanceBlur();
     }
     const effectTag = nextEffect.effectTag;
-    if ((effectTag & Snapshot) !== NoEffect) {
+    if ((effectTag & (Snapshot | ClearContainer)) !== NoEffect) {
       setCurrentDebugFiberInDEV(nextEffect);
 
       const current = nextEffect.alternate;

diff --git a/packages/react-reconciler/src/ReactInternalTypes.js b/packages/react-reconciler/src/ReactInternalTypes.js
@@ -233,9 +233,6 @@ type BaseFiberRootProperties = {|
   // render again
   lastPingedTime: ExpirationTime,
   lastExpiredTime: ExpirationTime,
-  // The container had non-React managed children before rendering;
-  // it should be cleared during the commit phase, before new children are appended.
-  clearContainerBeforeMount: boolean,
   // Used by useMutableSource hook to avoid tearing within this root
   // when external, mutable sources are read from during render.
   mutableSourceLastPendingUpdateTime: ExpirationTime,

diff --git a/packages/react-reconciler/src/ReactSideEffectTags.js b/packages/react-reconciler/src/ReactSideEffectTags.js
@@ -10,29 +10,30 @@
 export type SideEffectTag = number;
 
 // Don't change these two values. They're used by React Dev Tools.
-export const NoEffect = /*                 */ 0b00000000000000;
-export const PerformedWork = /*            */ 0b00000000000001;
+export const NoEffect = /*                 */ 0b000000000000000;
+export const PerformedWork = /*            */ 0b000000000000001;
 
 // You can change the rest (and add more).
-export const Placement = /*                */ 0b00000000000010;
-export const Update = /*                   */ 0b00000000000100;
-export const PlacementAndUpdate = /*       */ 0b00000000000110;
-export const Deletion = /*                 */ 0b00000000001000;
-export const ContentReset = /*             */ 0b00000000010000;
-export const Callback = /*                 */ 0b00000000100000;
-export const DidCapture = /*               */ 0b00000001000000;
-export const Ref = /*                      */ 0b00000010000000;
-export const Snapshot = /*                 */ 0b00000100000000;
-export const Passive = /*                  */ 0b00001000000000;
-export const PassiveUnmountPendingDev = /* */ 0b10000000000000;
-export const Hydrating = /*                */ 0b00010000000000;
-export const HydratingAndUpdate = /*       */ 0b00010000000100;
+export const Placement = /*                */ 0b000000000000010;
+export const Update = /*                   */ 0b000000000000100;
+export const PlacementAndUpdate = /*       */ 0b000000000000110;
+export const Deletion = /*                 */ 0b000000000001000;
+export const ContentReset = /*             */ 0b000000000010000;
+export const Callback = /*                 */ 0b000000000100000;
+export const DidCapture = /*               */ 0b000000001000000;
+export const Ref = /*                      */ 0b000000010000000;
+export const Snapshot = /*                 */ 0b000000100000000;
+export const Passive = /*                  */ 0b000001000000000;
+export const PassiveUnmountPendingDev = /* */ 0b010000000000000;
+export const Hydrating = /*                */ 0b000010000000000;
+export const HydratingAndUpdate = /*       */ 0b000010000000100;
+export const ClearContainer = /*           */ 0b100000000000000;
 
 // Passive & Update & Callback & Ref & Snapshot
-export const LifecycleEffectMask = /*      */ 0b00001110100100;
+export const LifecycleEffectMask = /*      */ 0b000001110100100;
 
 // Union of all host effects
-export const HostEffectMask = /*           */ 0b00011111111111;
+export const HostEffectMask = /*           */ 0b000011111111111;
 
-export const Incomplete = /*               */ 0b00100000000000;
-export const ShouldCapture = /*            */ 0b01000000000000;
+export const Incomplete = /*               */ 0b000100000000000;
+export const ShouldCapture = /*            */ 0b001000000000000;
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-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 {