Skip to content
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.

Commit 58d417b

Browse files
Brian Vaughnkoto
Brian Vaughn
authored and
koto
committedJun 15, 2021
Double Invoke Effects in __DEV__ (in old reconciler fork) (facebook#20415)
We originally added a new DEV behavior of double-invoking effects during mount to our new reconciler fork in PRs facebook#19523 and facebook#19935 and later refined it to only affect modern roots in PR facebook#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.
1 parent ff5d8d7 commit 58d417b

File tree

7 files changed

+1099
-50
lines changed

7 files changed

+1099
-50
lines changed
 

‎packages/react-reconciler/src/ReactFiberClassComponent.old.js‎

Lines changed: 39 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -12,13 +12,14 @@ import type {Lanes} from './ReactFiberLane.old';
1212
import type {UpdateQueue} from './ReactUpdateQueue.old';
1313

1414
import * as React from 'react';
15-
import {Update, Snapshot} from './ReactFiberFlags';
15+
import {Update, Snapshot, MountLayoutDev} from './ReactFiberFlags';
1616
import {
1717
debugRenderPhaseSideEffectsForStrictMode,
1818
disableLegacyContext,
1919
enableDebugTracing,
2020
enableSchedulingProfiler,
2121
warnAboutDeprecatedLifecycles,
22+
enableDoubleInvokingEffects,
2223
} from 'shared/ReactFeatureFlags';
2324
import ReactStrictModeWarnings from './ReactStrictModeWarnings.old';
2425
import {isMounted} from './ReactFiberTreeReflection';
@@ -29,7 +30,7 @@ import invariant from 'shared/invariant';
2930
import {REACT_CONTEXT_TYPE, REACT_PROVIDER_TYPE} from 'shared/ReactSymbols';
3031

3132
import {resolveDefaultProps} from './ReactFiberLazyComponent.old';
32-
import {DebugTracingMode, StrictMode} from './ReactTypeOfMode';
33+
import {DebugTracingMode, NoMode, StrictMode} from './ReactTypeOfMode';
3334

3435
import {
3536
enqueueUpdate,
@@ -890,7 +891,15 @@ function mountClassInstance(
890891
}
891892

892893
if (typeof instance.componentDidMount === 'function') {
893-
workInProgress.flags |= Update;
894+
if (
895+
__DEV__ &&
896+
enableDoubleInvokingEffects &&
897+
(workInProgress.mode & StrictMode) !== NoMode
898+
) {
899+
workInProgress.flags |= MountLayoutDev | Update;
900+
} else {
901+
workInProgress.flags |= Update;
902+
}
894903
}
895904
}
896905

@@ -960,7 +969,15 @@ function resumeMountClassInstance(
960969
// If an update was already in progress, we should schedule an Update
961970
// effect even though we're bailing out, so that cWU/cDU are called.
962971
if (typeof instance.componentDidMount === 'function') {
963-
workInProgress.flags |= Update;
972+
if (
973+
__DEV__ &&
974+
enableDoubleInvokingEffects &&
975+
(workInProgress.mode & StrictMode) !== NoMode
976+
) {
977+
workInProgress.flags |= MountLayoutDev | Update;
978+
} else {
979+
workInProgress.flags |= Update;
980+
}
964981
}
965982
return false;
966983
}
@@ -1003,13 +1020,29 @@ function resumeMountClassInstance(
10031020
}
10041021
}
10051022
if (typeof instance.componentDidMount === 'function') {
1006-
workInProgress.flags |= Update;
1023+
if (
1024+
__DEV__ &&
1025+
enableDoubleInvokingEffects &&
1026+
(workInProgress.mode & StrictMode) !== NoMode
1027+
) {
1028+
workInProgress.flags |= MountLayoutDev | Update;
1029+
} else {
1030+
workInProgress.flags |= Update;
1031+
}
10071032
}
10081033
} else {
10091034
// If an update was already in progress, we should schedule an Update
10101035
// effect even though we're bailing out, so that cWU/cDU are called.
10111036
if (typeof instance.componentDidMount === 'function') {
1012-
workInProgress.flags |= Update;
1037+
if (
1038+
__DEV__ &&
1039+
enableDoubleInvokingEffects &&
1040+
(workInProgress.mode & StrictMode) !== NoMode
1041+
) {
1042+
workInProgress.flags |= MountLayoutDev | Update;
1043+
} else {
1044+
workInProgress.flags |= Update;
1045+
}
10131046
}
10141047

10151048
// If shouldComponentUpdate returned false, we should still update the

‎packages/react-reconciler/src/ReactFiberCommitWork.old.js‎

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@ import {
3535
enableFundamentalAPI,
3636
enableSuspenseCallback,
3737
enableScopeAPI,
38+
enableDoubleInvokingEffects,
3839
} from 'shared/ReactFeatureFlags';
3940
import {
4041
FunctionComponent,
@@ -1798,6 +1799,116 @@ function commitResetTextContent(current: Fiber) {
17981799
resetTextContent(current.stateNode);
17991800
}
18001801

1802+
function invokeLayoutEffectMountInDEV(fiber: Fiber): void {
1803+
if (__DEV__ && enableDoubleInvokingEffects) {
1804+
switch (fiber.tag) {
1805+
case FunctionComponent:
1806+
case ForwardRef:
1807+
case SimpleMemoComponent: {
1808+
invokeGuardedCallback(
1809+
null,
1810+
commitHookEffectListMount,
1811+
null,
1812+
HookLayout | HookHasEffect,
1813+
fiber,
1814+
);
1815+
if (hasCaughtError()) {
1816+
const mountError = clearCaughtError();
1817+
captureCommitPhaseError(fiber, mountError);
1818+
}
1819+
break;
1820+
}
1821+
case ClassComponent: {
1822+
const instance = fiber.stateNode;
1823+
invokeGuardedCallback(null, instance.componentDidMount, instance);
1824+
if (hasCaughtError()) {
1825+
const mountError = clearCaughtError();
1826+
captureCommitPhaseError(fiber, mountError);
1827+
}
1828+
break;
1829+
}
1830+
}
1831+
}
1832+
}
1833+
1834+
function invokePassiveEffectMountInDEV(fiber: Fiber): void {
1835+
if (__DEV__ && enableDoubleInvokingEffects) {
1836+
switch (fiber.tag) {
1837+
case FunctionComponent:
1838+
case ForwardRef:
1839+
case SimpleMemoComponent: {
1840+
invokeGuardedCallback(
1841+
null,
1842+
commitHookEffectListMount,
1843+
null,
1844+
HookPassive | HookHasEffect,
1845+
fiber,
1846+
);
1847+
if (hasCaughtError()) {
1848+
const mountError = clearCaughtError();
1849+
captureCommitPhaseError(fiber, mountError);
1850+
}
1851+
break;
1852+
}
1853+
}
1854+
}
1855+
}
1856+
1857+
function invokeLayoutEffectUnmountInDEV(fiber: Fiber): void {
1858+
if (__DEV__ && enableDoubleInvokingEffects) {
1859+
switch (fiber.tag) {
1860+
case FunctionComponent:
1861+
case ForwardRef:
1862+
case SimpleMemoComponent: {
1863+
invokeGuardedCallback(
1864+
null,
1865+
commitHookEffectListUnmount,
1866+
null,
1867+
HookLayout | HookHasEffect,
1868+
fiber,
1869+
fiber.return,
1870+
);
1871+
if (hasCaughtError()) {
1872+
const unmountError = clearCaughtError();
1873+
captureCommitPhaseError(fiber, unmountError);
1874+
}
1875+
break;
1876+
}
1877+
case ClassComponent: {
1878+
const instance = fiber.stateNode;
1879+
if (typeof instance.componentWillUnmount === 'function') {
1880+
safelyCallComponentWillUnmount(fiber, instance);
1881+
}
1882+
break;
1883+
}
1884+
}
1885+
}
1886+
}
1887+
1888+
function invokePassiveEffectUnmountInDEV(fiber: Fiber): void {
1889+
if (__DEV__ && enableDoubleInvokingEffects) {
1890+
switch (fiber.tag) {
1891+
case FunctionComponent:
1892+
case ForwardRef:
1893+
case SimpleMemoComponent: {
1894+
invokeGuardedCallback(
1895+
null,
1896+
commitHookEffectListUnmount,
1897+
null,
1898+
HookPassive | HookHasEffect,
1899+
fiber,
1900+
fiber.return,
1901+
);
1902+
if (hasCaughtError()) {
1903+
const unmountError = clearCaughtError();
1904+
captureCommitPhaseError(fiber, unmountError);
1905+
}
1906+
break;
1907+
}
1908+
}
1909+
}
1910+
}
1911+
18011912
export {
18021913
commitBeforeMutationLifeCycles,
18031914
commitResetTextContent,
@@ -1807,4 +1918,8 @@ export {
18071918
commitLifeCycles,
18081919
commitAttachRef,
18091920
commitDetachRef,
1921+
invokeLayoutEffectMountInDEV,
1922+
invokeLayoutEffectUnmountInDEV,
1923+
invokePassiveEffectMountInDEV,
1924+
invokePassiveEffectUnmountInDEV,
18101925
};

‎packages/react-reconciler/src/ReactFiberHooks.old.js‎

Lines changed: 87 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -28,9 +28,15 @@ import {
2828
enableCache,
2929
decoupleUpdatePriorityFromScheduler,
3030
enableUseRefAccessWarning,
31+
enableDoubleInvokingEffects,
3132
} from 'shared/ReactFeatureFlags';
3233

33-
import {NoMode, BlockingMode, DebugTracingMode} from './ReactTypeOfMode';
34+
import {
35+
NoMode,
36+
BlockingMode,
37+
DebugTracingMode,
38+
StrictMode,
39+
} from './ReactTypeOfMode';
3440
import {
3541
NoLane,
3642
NoLanes,
@@ -49,6 +55,8 @@ import {readContext} from './ReactFiberNewContext.old';
4955
import {
5056
Update as UpdateEffect,
5157
Passive as PassiveEffect,
58+
MountLayoutDev as MountLayoutDevEffect,
59+
MountPassiveDev as MountPassiveDevEffect,
5260
} from './ReactFiberFlags';
5361
import {
5462
HasEffect as HookHasEffect,
@@ -467,7 +475,20 @@ export function bailoutHooks(
467475
lanes: Lanes,
468476
) {
469477
workInProgress.updateQueue = current.updateQueue;
470-
workInProgress.flags &= ~(PassiveEffect | UpdateEffect);
478+
if (
479+
__DEV__ &&
480+
enableDoubleInvokingEffects &&
481+
(workInProgress.mode & StrictMode) !== NoMode
482+
) {
483+
workInProgress.flags &= ~(
484+
MountLayoutDevEffect |
485+
MountPassiveDevEffect |
486+
PassiveEffect |
487+
UpdateEffect
488+
);
489+
} else {
490+
workInProgress.flags &= ~(PassiveEffect | UpdateEffect);
491+
}
471492
current.lanes = removeLanes(current.lanes, lanes);
472493
}
473494

@@ -1303,12 +1324,26 @@ function mountEffect(
13031324
warnIfNotCurrentlyActingEffectsInDEV(currentlyRenderingFiber);
13041325
}
13051326
}
1306-
return mountEffectImpl(
1307-
UpdateEffect | PassiveEffect,
1308-
HookPassive,
1309-
create,
1310-
deps,
1311-
);
1327+
1328+
if (
1329+
__DEV__ &&
1330+
enableDoubleInvokingEffects &&
1331+
(currentlyRenderingFiber.mode & StrictMode) !== NoMode
1332+
) {
1333+
return mountEffectImpl(
1334+
UpdateEffect | PassiveEffect | MountPassiveDevEffect,
1335+
HookPassive,
1336+
create,
1337+
deps,
1338+
);
1339+
} else {
1340+
return mountEffectImpl(
1341+
UpdateEffect | PassiveEffect,
1342+
HookPassive,
1343+
create,
1344+
deps,
1345+
);
1346+
}
13121347
}
13131348

13141349
function updateEffect(
@@ -1333,7 +1368,20 @@ function mountLayoutEffect(
13331368
create: () => (() => void) | void,
13341369
deps: Array<mixed> | void | null,
13351370
): void {
1336-
return mountEffectImpl(UpdateEffect, HookLayout, create, deps);
1371+
if (
1372+
__DEV__ &&
1373+
enableDoubleInvokingEffects &&
1374+
(currentlyRenderingFiber.mode & StrictMode) !== NoMode
1375+
) {
1376+
return mountEffectImpl(
1377+
UpdateEffect | MountLayoutDevEffect,
1378+
HookLayout,
1379+
create,
1380+
deps,
1381+
);
1382+
} else {
1383+
return mountEffectImpl(UpdateEffect, HookLayout, create, deps);
1384+
}
13371385
}
13381386

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

1395-
return mountEffectImpl(
1396-
UpdateEffect,
1397-
HookLayout,
1398-
imperativeHandleEffect.bind(null, create, ref),
1399-
effectDeps,
1400-
);
1443+
if (
1444+
__DEV__ &&
1445+
enableDoubleInvokingEffects &&
1446+
(currentlyRenderingFiber.mode & StrictMode) !== NoMode
1447+
) {
1448+
return mountEffectImpl(
1449+
UpdateEffect | MountLayoutDevEffect,
1450+
HookLayout,
1451+
imperativeHandleEffect.bind(null, create, ref),
1452+
effectDeps,
1453+
);
1454+
} else {
1455+
return mountEffectImpl(
1456+
UpdateEffect,
1457+
HookLayout,
1458+
imperativeHandleEffect.bind(null, create, ref),
1459+
effectDeps,
1460+
);
1461+
}
14011462
}
14021463

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

16801741
if ((currentlyRenderingFiber.mode & BlockingMode) === NoMode) {
1681-
currentlyRenderingFiber.flags |= UpdateEffect | PassiveEffect;
1742+
if (
1743+
__DEV__ &&
1744+
enableDoubleInvokingEffects &&
1745+
(currentlyRenderingFiber.mode & StrictMode) !== NoMode
1746+
) {
1747+
currentlyRenderingFiber.flags |=
1748+
UpdateEffect | PassiveEffect | MountPassiveDevEffect;
1749+
} else {
1750+
currentlyRenderingFiber.flags |= UpdateEffect | PassiveEffect;
1751+
}
1752+
16821753
pushEffect(
16831754
HookHasEffect | HookPassive,
16841755
() => {

‎packages/react-reconciler/src/ReactFiberWorkLoop.old.js‎

Lines changed: 80 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import type {ReactPriorityLevel} from './ReactInternalTypes';
1414
import type {Interaction} from 'scheduler/src/Tracing';
1515
import type {SuspenseState} from './ReactFiberSuspenseComponent.old';
1616
import type {Effect as HookEffect} from './ReactFiberHooks.old';
17+
import type {Flags} from './ReactFiberFlags';
1718
import type {StackCursor} from './ReactFiberStack.old';
1819

1920
import {
@@ -31,6 +32,7 @@ import {
3132
enableDebugTracing,
3233
enableSchedulingProfiler,
3334
enableScopeAPI,
35+
enableDoubleInvokingEffects,
3436
} from 'shared/ReactFeatureFlags';
3537
import ReactSharedInternals from 'shared/ReactSharedInternals';
3638
import invariant from 'shared/invariant';
@@ -131,6 +133,8 @@ import {
131133
HostEffectMask,
132134
Hydrating,
133135
HydratingAndUpdate,
136+
MountPassiveDev,
137+
MountLayoutDev,
134138
} from './ReactFiberFlags';
135139
import {
136140
NoLanePriority,
@@ -191,6 +195,10 @@ import {
191195
commitPassiveEffectDurations,
192196
commitResetTextContent,
193197
isSuspenseBoundaryBeingHidden,
198+
invokeLayoutEffectMountInDEV,
199+
invokePassiveEffectMountInDEV,
200+
invokeLayoutEffectUnmountInDEV,
201+
invokePassiveEffectUnmountInDEV,
194202
} from './ReactFiberCommitWork.old';
195203
import {enqueueUpdate} from './ReactUpdateQueue.old';
196204
import {resetContextDependencies} from './ReactFiberNewContext.old';
@@ -2113,6 +2121,10 @@ function commitRootImpl(root, renderPriorityLevel) {
21132121
pendingPassiveEffectsLanes = lanes;
21142122
pendingPassiveEffectsRenderPriority = renderPriorityLevel;
21152123
} else {
2124+
if (__DEV__ && enableDoubleInvokingEffects) {
2125+
commitDoubleInvokeEffectsInDEV(root, false);
2126+
}
2127+
21162128
// We are done with the effect chain at this point so let's clear the
21172129
// nextEffect pointers to assist with GC. If we have passive effects, we'll
21182130
// clear this in flushPassiveEffects.
@@ -2660,6 +2672,29 @@ function flushPassiveEffectsImpl() {
26602672
}
26612673
}
26622674

2675+
if (enableProfilerTimer && enableProfilerCommitHooks) {
2676+
const profilerEffects = pendingPassiveProfilerEffects;
2677+
pendingPassiveProfilerEffects = [];
2678+
for (let i = 0; i < profilerEffects.length; i++) {
2679+
const fiber = ((profilerEffects[i]: any): Fiber);
2680+
commitPassiveEffectDurations(root, fiber);
2681+
}
2682+
}
2683+
2684+
if (__DEV__) {
2685+
if (enableDebugTracing) {
2686+
logPassiveEffectsStopped();
2687+
}
2688+
}
2689+
2690+
if (enableSchedulingProfiler) {
2691+
markPassiveEffectsStopped();
2692+
}
2693+
2694+
if (__DEV__ && enableDoubleInvokingEffects) {
2695+
commitDoubleInvokeEffectsInDEV(root, true);
2696+
}
2697+
26632698
// Note: This currently assumes there are no passive effects on the root fiber
26642699
// because the root is not part of its own effect list.
26652700
// This could change in the future.
@@ -2674,34 +2709,15 @@ function flushPassiveEffectsImpl() {
26742709
effect = nextNextEffect;
26752710
}
26762711

2677-
if (enableProfilerTimer && enableProfilerCommitHooks) {
2678-
const profilerEffects = pendingPassiveProfilerEffects;
2679-
pendingPassiveProfilerEffects = [];
2680-
for (let i = 0; i < profilerEffects.length; i++) {
2681-
const fiber = ((profilerEffects[i]: any): Fiber);
2682-
commitPassiveEffectDurations(root, fiber);
2683-
}
2712+
if (__DEV__) {
2713+
isFlushingPassiveEffects = false;
26842714
}
26852715

26862716
if (enableSchedulerTracing) {
26872717
popInteractions(((prevInteractions: any): Set<Interaction>));
26882718
finishPendingInteractions(root, lanes);
26892719
}
26902720

2691-
if (__DEV__) {
2692-
isFlushingPassiveEffects = false;
2693-
}
2694-
2695-
if (__DEV__) {
2696-
if (enableDebugTracing) {
2697-
logPassiveEffectsStopped();
2698-
}
2699-
}
2700-
2701-
if (enableSchedulingProfiler) {
2702-
markPassiveEffectsStopped();
2703-
}
2704-
27052721
executionContext = prevExecutionContext;
27062722

27072723
flushSyncCallbackQueue();
@@ -2985,6 +3001,49 @@ function flushRenderPhaseStrictModeWarningsInDEV() {
29853001
}
29863002
}
29873003

3004+
function commitDoubleInvokeEffectsInDEV(
3005+
root: FiberRoot,
3006+
hasPassiveEffects: boolean,
3007+
) {
3008+
if (__DEV__ && enableDoubleInvokingEffects) {
3009+
invokeEffectsInDev(root, MountLayoutDev, invokeLayoutEffectUnmountInDEV);
3010+
if (hasPassiveEffects) {
3011+
invokeEffectsInDev(
3012+
root,
3013+
MountPassiveDev,
3014+
invokePassiveEffectUnmountInDEV,
3015+
);
3016+
}
3017+
3018+
invokeEffectsInDev(root, MountLayoutDev, invokeLayoutEffectMountInDEV);
3019+
if (hasPassiveEffects) {
3020+
invokeEffectsInDev(root, MountPassiveDev, invokePassiveEffectMountInDEV);
3021+
}
3022+
}
3023+
}
3024+
3025+
function invokeEffectsInDev(
3026+
root: FiberRoot,
3027+
fiberFlags: Flags,
3028+
invokeEffectFn: (fiber: Fiber) => void,
3029+
): void {
3030+
if (__DEV__ && enableDoubleInvokingEffects) {
3031+
// Note: This currently assumes there are no passive effects on the root fiber
3032+
// because the root is not part of its own effect list.
3033+
// This could change in the future.
3034+
let currentEffect = root.current.firstEffect;
3035+
while (currentEffect !== null) {
3036+
const flags = currentEffect.flags;
3037+
if ((flags & fiberFlags) !== NoFlags) {
3038+
setCurrentDebugFiberInDEV(currentEffect);
3039+
invokeEffectFn(currentEffect);
3040+
resetCurrentDebugFiberInDEV();
3041+
}
3042+
currentEffect = currentEffect.nextEffect;
3043+
}
3044+
}
3045+
}
3046+
29883047
let didWarnStateUpdateForNotYetMountedComponent: Set<string> | null = null;
29893048
function warnAboutUpdateOnNotYetMountedFiberInDEV(fiber) {
29903049
if (__DEV__) {

‎packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.internal.js‎

Lines changed: 771 additions & 0 deletions
Large diffs are not rendered by default.

‎packages/react-reconciler/src/__tests__/ReactDoubleInvokeEvents-test.js‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,7 @@ describe('ReactDoubleInvokeEvents', () => {
2626
function supportsDoubleInvokeEffects() {
2727
return gate(
2828
flags =>
29-
flags.build === 'development' &&
30-
flags.enableDoubleInvokingEffects &&
31-
flags.dfsEffectsRefactor,
29+
flags.build === 'development' && flags.enableDoubleInvokingEffects,
3230
);
3331
}
3432

‎packages/react/src/__tests__/ReactProfiler-test.internal.js‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4874,10 +4874,8 @@ describe('Profiler', () => {
48744874
});
48754875

48764876
if (__DEV__) {
4877-
// @gate dfsEffectsRefactor
4878-
// @gate enableDoubleInvokingEffects
48794877
it('double invoking does not disconnect wrapped async work', () => {
4880-
ReactFeatureFlags.enableDoubleInvokingEffects = true;
4878+
ReactFeatureFlags.enableDoubleInvokingEffects = !__VARIANT__;
48814879

48824880
const callback = jest.fn(() => {
48834881
const wrappedInteractions = SchedulerTracing.unstable_getCurrent();
@@ -4915,7 +4913,11 @@ describe('Profiler', () => {
49154913

49164914
jest.runAllTimers();
49174915

4918-
expect(callback).toHaveBeenCalledTimes(4); // 2x per effect
4916+
if (ReactFeatureFlags.enableDoubleInvokingEffects) {
4917+
expect(callback).toHaveBeenCalledTimes(4); // 2x per effect
4918+
} else {
4919+
expect(callback).toHaveBeenCalledTimes(2);
4920+
}
49194921

49204922
expect(onInteractionScheduledWorkCompleted).toHaveBeenCalledTimes(1);
49214923
});

0 commit comments

Comments
 (0)
Please sign in to comment.