Skip to content

Commit

Permalink
Don't double-invoke effects in legacy roots (#20028)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Brian Vaughn authored Oct 15, 2020
1 parent d95c493 commit 02da938
Show file tree
Hide file tree
Showing 6 changed files with 193 additions and 56 deletions.
33 changes: 28 additions & 5 deletions packages/react-reconciler/src/ReactFiberClassComponent.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand Down
8 changes: 8 additions & 0 deletions packages/react-reconciler/src/ReactFiberCommitWork.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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:
Expand All @@ -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:
Expand Down Expand Up @@ -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:
Expand Down
31 changes: 26 additions & 5 deletions packages/react-reconciler/src/ReactFiberHooks.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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 |
Expand Down Expand Up @@ -1253,7 +1262,11 @@ function mountEffect(
}
}

if (__DEV__ && enableDoubleInvokingEffects) {
if (
__DEV__ &&
enableDoubleInvokingEffects &&
(currentlyRenderingFiber.mode & (BlockingMode | ConcurrentMode)) !== NoMode
) {
return mountEffectImpl(
MountPassiveDevEffect | PassiveEffect | PassiveStaticEffect,
HookPassive,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
7 changes: 7 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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) {
Expand Down
Loading

0 comments on commit 02da938

Please sign in to comment.