Don't double-invoke effects in legacy roots

Large legacy applications are likely to be difficult to update to handle this feature, and it wouldn't add any value– since newer APIs that require this resilience are not legacy compatible.
facebook · Oct 14, 2020 · 43eab00 · 43eab00
1 parent 4eb5891
commit 43eab00
Show file tree

Hide file tree

Showing 6 changed files with 193 additions and 56 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberClassComponent.new.js b/packages/react-reconciler/src/ReactFiberClassComponent.new.js
@@ -30,7 +30,13 @@ import invariant from 'shared/invariant';
 import {REACT_CONTEXT_TYPE, REACT_PROVIDER_TYPE} from 'shared/ReactSymbols';
 
 import {resolveDefaultProps} from './ReactFiberLazyComponent.new';
-import {DebugTracingMode, StrictMode} from './ReactTypeOfMode';
+import {
+  BlockingMode,
+  ConcurrentMode,
+  DebugTracingMode,
+  NoMode,
+  StrictMode,
+} from './ReactTypeOfMode';
 
 import {
   enqueueUpdate,
@@ -891,7 +897,12 @@ function mountClassInstance(
   }
 
   if (typeof instance.componentDidMount === 'function') {
-    if (__DEV__ && enableDoubleInvokingEffects) {
+    if (
+      __DEV__ &&
+      enableDoubleInvokingEffects &&
+      (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode
+    ) {
+      // Never double-invoke effects for legacy roots.
       workInProgress.flags |= MountLayoutDev | Update;
     } else {
       workInProgress.flags |= Update;
@@ -965,7 +976,11 @@ function resumeMountClassInstance(
     // If an update was already in progress, we should schedule an Update
     // effect even though we're bailing out, so that cWU/cDU are called.
     if (typeof instance.componentDidMount === 'function') {
-      if (__DEV__ && enableDoubleInvokingEffects) {
+      if (
+        __DEV__ &&
+        enableDoubleInvokingEffects &&
+        (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode
+      ) {
         workInProgress.flags |= MountLayoutDev | Update;
       } else {
         workInProgress.flags |= Update;
@@ -1012,7 +1027,11 @@ function resumeMountClassInstance(
       }
     }
     if (typeof instance.componentDidMount === 'function') {
-      if (__DEV__ && enableDoubleInvokingEffects) {
+      if (
+        __DEV__ &&
+        enableDoubleInvokingEffects &&
+        (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode
+      ) {
         workInProgress.flags |= MountLayoutDev | Update;
       } else {
         workInProgress.flags |= Update;
@@ -1022,7 +1041,11 @@ function resumeMountClassInstance(
     // If an update was already in progress, we should schedule an Update
     // effect even though we're bailing out, so that cWU/cDU are called.
     if (typeof instance.componentDidMount === 'function') {
-      if (__DEV__ && enableDoubleInvokingEffects) {
+      if (
+        __DEV__ &&
+        enableDoubleInvokingEffects &&
+        (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode
+      ) {
         workInProgress.flags |= MountLayoutDev | Update;
       } else {
         workInProgress.flags |= Update;

diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -2024,6 +2024,8 @@ function commitPassiveMount(
 
 function invokeLayoutEffectMountInDEV(fiber: Fiber): void {
   if (__DEV__ && enableDoubleInvokingEffects) {
+    // We don't need to re-check for legacy roots here.
+    // This function will not be called within legacy roots.
     switch (fiber.tag) {
       case FunctionComponent:
       case ForwardRef:
@@ -2057,6 +2059,8 @@ function invokeLayoutEffectMountInDEV(fiber: Fiber): void {
 
 function invokePassiveEffectMountInDEV(fiber: Fiber): void {
   if (__DEV__ && enableDoubleInvokingEffects) {
+    // We don't need to re-check for legacy roots here.
+    // This function will not be called within legacy roots.
     switch (fiber.tag) {
       case FunctionComponent:
       case ForwardRef:
@@ -2081,6 +2085,8 @@ function invokePassiveEffectMountInDEV(fiber: Fiber): void {
 
 function invokeLayoutEffectUnmountInDEV(fiber: Fiber): void {
   if (__DEV__ && enableDoubleInvokingEffects) {
+    // We don't need to re-check for legacy roots here.
+    // This function will not be called within legacy roots.
     switch (fiber.tag) {
       case FunctionComponent:
       case ForwardRef:
@@ -2113,6 +2119,8 @@ function invokeLayoutEffectUnmountInDEV(fiber: Fiber): void {
 
 function invokePassiveEffectUnmountInDEV(fiber: Fiber): void {
   if (__DEV__ && enableDoubleInvokingEffects) {
+    // We don't need to re-check for legacy roots here.
+    // This function will not be called within legacy roots.
     switch (fiber.tag) {
       case FunctionComponent:
       case ForwardRef:

diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js
@@ -29,7 +29,12 @@ import {
   enableDoubleInvokingEffects,
 } from 'shared/ReactFeatureFlags';
 
-import {NoMode, BlockingMode, DebugTracingMode} from './ReactTypeOfMode';
+import {
+  NoMode,
+  BlockingMode,
+  ConcurrentMode,
+  DebugTracingMode,
+} from './ReactTypeOfMode';
 import {
   NoLane,
   NoLanes,
@@ -485,7 +490,11 @@ export function bailoutHooks(
   lanes: Lanes,
 ) {
   workInProgress.updateQueue = current.updateQueue;
-  if (__DEV__ && enableDoubleInvokingEffects) {
+  if (
+    __DEV__ &&
+    enableDoubleInvokingEffects &&
+    (workInProgress.mode & (BlockingMode | ConcurrentMode)) !== NoMode
+  ) {
     workInProgress.flags &= ~(
       MountPassiveDevEffect |
       PassiveEffect |
@@ -1253,7 +1262,11 @@ function mountEffect(
     }
   }
 
-  if (__DEV__ && enableDoubleInvokingEffects) {
+  if (
+    __DEV__ &&
+    enableDoubleInvokingEffects &&
+    (currentlyRenderingFiber.mode & (BlockingMode | ConcurrentMode)) !== NoMode
+  ) {
     return mountEffectImpl(
       MountPassiveDevEffect | PassiveEffect | PassiveStaticEffect,
       HookPassive,
@@ -1287,7 +1300,11 @@ function mountLayoutEffect(
   create: () => (() => void) | void,
   deps: Array<mixed> | void | null,
 ): void {
-  if (__DEV__ && enableDoubleInvokingEffects) {
+  if (
+    __DEV__ &&
+    enableDoubleInvokingEffects &&
+    (currentlyRenderingFiber.mode & (BlockingMode | ConcurrentMode)) !== NoMode
+  ) {
     return mountEffectImpl(
       MountLayoutDevEffect | UpdateEffect,
       HookLayout,
@@ -1355,7 +1372,11 @@ function mountImperativeHandle<T>(
   const effectDeps =
     deps !== null && deps !== undefined ? deps.concat([ref]) : null;
 
-  if (__DEV__ && enableDoubleInvokingEffects) {
+  if (
+    __DEV__ &&
+    enableDoubleInvokingEffects &&
+    (currentlyRenderingFiber.mode & (BlockingMode | ConcurrentMode)) !== NoMode
+  ) {
     return mountEffectImpl(
       MountLayoutDevEffect | UpdateEffect,
       HookLayout,

diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -2874,6 +2874,11 @@ function commitDoubleInvokeEffectsInDEV(
   hasPassiveEffects: boolean,
 ) {
   if (__DEV__ && enableDoubleInvokingEffects) {
+    // Never double-invoke effects for legacy roots.
+    if ((fiber.mode & (BlockingMode | ConcurrentMode)) === NoMode) {
+      return;
+    }
+
     setCurrentDebugFiberInDEV(fiber);
     invokeEffectsInDev(fiber, MountLayoutDev, invokeLayoutEffectUnmountInDEV);
     if (hasPassiveEffects) {
@@ -2898,6 +2903,8 @@ function invokeEffectsInDev(
   invokeEffectFn: (fiber: Fiber) => void,
 ): void {
   if (__DEV__ && enableDoubleInvokingEffects) {
+    // We don't need to re-check for legacy roots here.
+    // This function will not be called within legacy roots.
     let fiber = firstChild;
     while (fiber !== null) {
       if (fiber.child !== null) {