Skip to content

Commit

Permalink
Double Invoke Effects in __DEV__ (in old reconciler fork) (#20415)
Browse files Browse the repository at this point in the history 
We originally added a new DEV behavior of double-invoking effects during mount to our new reconciler fork in PRs #19523 and #19935 and later refined it to only affect modern roots in PR #20028. This PR adds that behavior to the old reconciler fork with a small twist– the behavior applies to StrictMode subtrees, regardless of the root type.

This commit also adds a few additional tests that weren't in the original commits.
  • Loading branch information
Brian Vaughn authored Dec 9, 2020
1 parent 1a24223 commit 56a632a
Show file tree
Hide file tree
Showing 7 changed files with 1,099 additions and 50 deletions.
45 changes: 39 additions & 6 deletions packages/react-reconciler/src/ReactFiberClassComponent.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,14 @@ import type {Lanes} from './ReactFiberLane.old';
import type {UpdateQueue} from './ReactUpdateQueue.old';

import * as React from 'react';
import {Update, Snapshot} from './ReactFiberFlags';
import {Update, Snapshot, MountLayoutDev} from './ReactFiberFlags';
import {
debugRenderPhaseSideEffectsForStrictMode,
disableLegacyContext,
enableDebugTracing,
enableSchedulingProfiler,
warnAboutDeprecatedLifecycles,
enableDoubleInvokingEffects,
} from 'shared/ReactFeatureFlags';
import ReactStrictModeWarnings from './ReactStrictModeWarnings.old';
import {isMounted} from './ReactFiberTreeReflection';
Expand All @@ -29,7 +30,7 @@ import invariant from 'shared/invariant';
import {REACT_CONTEXT_TYPE, REACT_PROVIDER_TYPE} from 'shared/ReactSymbols';

import {resolveDefaultProps} from './ReactFiberLazyComponent.old';
import {DebugTracingMode, StrictMode} from './ReactTypeOfMode';
import {DebugTracingMode, NoMode, StrictMode} from './ReactTypeOfMode';

import {
enqueueUpdate,
Expand Down Expand Up @@ -890,7 +891,15 @@ function mountClassInstance(
}

if (typeof instance.componentDidMount === 'function') {
workInProgress.flags |= Update;
if (
__DEV__ &&
enableDoubleInvokingEffects &&
(workInProgress.mode & StrictMode) !== NoMode
) {
workInProgress.flags |= MountLayoutDev | Update;
} else {
workInProgress.flags |= Update;
}
}
}

Expand Down Expand Up @@ -960,7 +969,15 @@ 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') {
workInProgress.flags |= Update;
if (
__DEV__ &&
enableDoubleInvokingEffects &&
(workInProgress.mode & StrictMode) !== NoMode
) {
workInProgress.flags |= MountLayoutDev | Update;
} else {
workInProgress.flags |= Update;
}
}
return false;
}
Expand Down Expand Up @@ -1003,13 +1020,29 @@ function resumeMountClassInstance(
}
}
if (typeof instance.componentDidMount === 'function') {
workInProgress.flags |= Update;
if (
__DEV__ &&
enableDoubleInvokingEffects &&
(workInProgress.mode & StrictMode) !== NoMode
) {
workInProgress.flags |= MountLayoutDev | Update;
} else {
workInProgress.flags |= Update;
}
}
} else {
// 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') {
workInProgress.flags |= Update;
if (
__DEV__ &&
enableDoubleInvokingEffects &&
(workInProgress.mode & StrictMode) !== NoMode
) {
workInProgress.flags |= MountLayoutDev | Update;
} else {
workInProgress.flags |= Update;
}
}

// If shouldComponentUpdate returned false, we should still update the
Expand Down
115 changes: 115 additions & 0 deletions packages/react-reconciler/src/ReactFiberCommitWork.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ import {
enableFundamentalAPI,
enableSuspenseCallback,
enableScopeAPI,
enableDoubleInvokingEffects,
} from 'shared/ReactFeatureFlags';
import {
FunctionComponent,
Expand Down Expand Up @@ -1798,6 +1799,116 @@ function commitResetTextContent(current: Fiber) {
resetTextContent(current.stateNode);
}

function invokeLayoutEffectMountInDEV(fiber: Fiber): void {
if (__DEV__ && enableDoubleInvokingEffects) {
switch (fiber.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent: {
invokeGuardedCallback(
null,
commitHookEffectListMount,
null,
HookLayout | HookHasEffect,
fiber,
);
if (hasCaughtError()) {
const mountError = clearCaughtError();
captureCommitPhaseError(fiber, mountError);
}
break;
}
case ClassComponent: {
const instance = fiber.stateNode;
invokeGuardedCallback(null, instance.componentDidMount, instance);
if (hasCaughtError()) {
const mountError = clearCaughtError();
captureCommitPhaseError(fiber, mountError);
}
break;
}
}
}
}

function invokePassiveEffectMountInDEV(fiber: Fiber): void {
if (__DEV__ && enableDoubleInvokingEffects) {
switch (fiber.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent: {
invokeGuardedCallback(
null,
commitHookEffectListMount,
null,
HookPassive | HookHasEffect,
fiber,
);
if (hasCaughtError()) {
const mountError = clearCaughtError();
captureCommitPhaseError(fiber, mountError);
}
break;
}
}
}
}

function invokeLayoutEffectUnmountInDEV(fiber: Fiber): void {
if (__DEV__ && enableDoubleInvokingEffects) {
switch (fiber.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent: {
invokeGuardedCallback(
null,
commitHookEffectListUnmount,
null,
HookLayout | HookHasEffect,
fiber,
fiber.return,
);
if (hasCaughtError()) {
const unmountError = clearCaughtError();
captureCommitPhaseError(fiber, unmountError);
}
break;
}
case ClassComponent: {
const instance = fiber.stateNode;
if (typeof instance.componentWillUnmount === 'function') {
safelyCallComponentWillUnmount(fiber, instance);
}
break;
}
}
}
}

function invokePassiveEffectUnmountInDEV(fiber: Fiber): void {
if (__DEV__ && enableDoubleInvokingEffects) {
switch (fiber.tag) {
case FunctionComponent:
case ForwardRef:
case SimpleMemoComponent: {
invokeGuardedCallback(
null,
commitHookEffectListUnmount,
null,
HookPassive | HookHasEffect,
fiber,
fiber.return,
);
if (hasCaughtError()) {
const unmountError = clearCaughtError();
captureCommitPhaseError(fiber, unmountError);
}
break;
}
}
}
}

export {
commitBeforeMutationLifeCycles,
commitResetTextContent,
Expand All @@ -1807,4 +1918,8 @@ export {
commitLifeCycles,
commitAttachRef,
commitDetachRef,
invokeLayoutEffectMountInDEV,
invokeLayoutEffectUnmountInDEV,
invokePassiveEffectMountInDEV,
invokePassiveEffectUnmountInDEV,
};
103 changes: 87 additions & 16 deletions packages/react-reconciler/src/ReactFiberHooks.old.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,15 @@ import {
enableCache,
decoupleUpdatePriorityFromScheduler,
enableUseRefAccessWarning,
enableDoubleInvokingEffects,
} from 'shared/ReactFeatureFlags';

import {NoMode, BlockingMode, DebugTracingMode} from './ReactTypeOfMode';
import {
NoMode,
BlockingMode,
DebugTracingMode,
StrictMode,
} from './ReactTypeOfMode';
import {
NoLane,
NoLanes,
Expand All @@ -49,6 +55,8 @@ import {readContext} from './ReactFiberNewContext.old';
import {
Update as UpdateEffect,
Passive as PassiveEffect,
MountLayoutDev as MountLayoutDevEffect,
MountPassiveDev as MountPassiveDevEffect,
} from './ReactFiberFlags';
import {
HasEffect as HookHasEffect,
Expand Down Expand Up @@ -467,7 +475,20 @@ export function bailoutHooks(
lanes: Lanes,
) {
workInProgress.updateQueue = current.updateQueue;
workInProgress.flags &= ~(PassiveEffect | UpdateEffect);
if (
__DEV__ &&
enableDoubleInvokingEffects &&
(workInProgress.mode & StrictMode) !== NoMode
) {
workInProgress.flags &= ~(
MountLayoutDevEffect |
MountPassiveDevEffect |
PassiveEffect |
UpdateEffect
);
} else {
workInProgress.flags &= ~(PassiveEffect | UpdateEffect);
}
current.lanes = removeLanes(current.lanes, lanes);
}

Expand Down Expand Up @@ -1303,12 +1324,26 @@ function mountEffect(
warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber);
}
}
return mountEffectImpl(
UpdateEffect | PassiveEffect,
HookPassive,
create,
deps,
);

if (
__DEV__ &&
enableDoubleInvokingEffects &&
(currentlyRenderingFiber.mode & StrictMode) !== NoMode
) {
return mountEffectImpl(
UpdateEffect | PassiveEffect | MountPassiveDevEffect,
HookPassive,
create,
deps,
);
} else {
return mountEffectImpl(
UpdateEffect | PassiveEffect,
HookPassive,
create,
deps,
);
}
}

function updateEffect(
Expand All @@ -1333,7 +1368,20 @@ function mountLayoutEffect(
create: () => (() => void) | void,
deps: Array<mixed> | void | null,
): void {
return mountEffectImpl(UpdateEffect, HookLayout, create, deps);
if (
__DEV__ &&
enableDoubleInvokingEffects &&
(currentlyRenderingFiber.mode & StrictMode) !== NoMode
) {
return mountEffectImpl(
UpdateEffect | MountLayoutDevEffect,
HookLayout,
create,
deps,
);
} else {
return mountEffectImpl(UpdateEffect, HookLayout, create, deps);
}
}

function updateLayoutEffect(
Expand Down Expand Up @@ -1392,12 +1440,25 @@ function mountImperativeHandle<T>(
const effectDeps =
deps !== null && deps !== undefined ? deps.concat([ref]) : null;

return mountEffectImpl(
UpdateEffect,
HookLayout,
imperativeHandleEffect.bind(null, create, ref),
effectDeps,
);
if (
__DEV__ &&
enableDoubleInvokingEffects &&
(currentlyRenderingFiber.mode & StrictMode) !== NoMode
) {
return mountEffectImpl(
UpdateEffect | MountLayoutDevEffect,
HookLayout,
imperativeHandleEffect.bind(null, create, ref),
effectDeps,
);
} else {
return mountEffectImpl(
UpdateEffect,
HookLayout,
imperativeHandleEffect.bind(null, create, ref),
effectDeps,
);
}
}

function updateImperativeHandle<T>(
Expand Down Expand Up @@ -1678,7 +1739,17 @@ function mountOpaqueIdentifier(): OpaqueIDType | void {
const setId = mountState(id)[1];

if ((currentlyRenderingFiber.mode & BlockingMode) === NoMode) {
currentlyRenderingFiber.flags |= UpdateEffect | PassiveEffect;
if (
__DEV__ &&
enableDoubleInvokingEffects &&
(currentlyRenderingFiber.mode & StrictMode) !== NoMode
) {
currentlyRenderingFiber.flags |=
UpdateEffect | PassiveEffect | MountPassiveDevEffect;
} else {
currentlyRenderingFiber.flags |= UpdateEffect | PassiveEffect;
}

pushEffect(
HookHasEffect | HookPassive,
() => {
Expand Down
Loading

0 comments on commit 56a632a

Please sign in to comment.