From b58e231cc9122e559c225125896ab4ae1769a79a Mon Sep 17 00:00:00 2001
From: Dan Abramov <dan.abramov@gmail.com>
Date: Thu, 7 Apr 2022 18:54:44 +0100
Subject: [PATCH 1/2] Warn on setState() in useInsertionEffect()
---
.../src/ReactFiberCommitWork.new.js | 23 +++++++++++-
.../src/ReactFiberCommitWork.old.js | 23 +++++++++++-
.../src/ReactFiberWorkLoop.new.js | 14 +++++++
.../src/ReactFiberWorkLoop.old.js | 14 +++++++
.../ReactHooksWithNoopRenderer-test.js | 37 +++++++++++++++++++
5 files changed, 107 insertions(+), 4 deletions(-)
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
index fb60401095184..aad83fa42c8c2 100644
--- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js
+++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -136,6 +136,7 @@ import {
restorePendingUpdaters,
addTransitionStartCallbackToPendingTransition,
addTransitionCompleteCallbackToPendingTransition,
+ setIsRunningInsertionEffect,
} from './ReactFiberWorkLoop.new';
import {
NoFlags as NoHookEffect,
@@ -536,7 +537,16 @@ function commitHookEffectListUnmount(
}
}
- safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy);
+ if (__DEV__ && (flags & HookInsertion) !== NoHookEffect) {
+ setIsRunningInsertionEffect(true);
+ try {
+ safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy);
+ } finally {
+ setIsRunningInsertionEffect(false);
+ }
+ } else {
+ safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy);
+ }
if (enableSchedulingProfiler) {
if ((flags & HookPassive) !== NoHookEffect) {
@@ -570,7 +580,16 @@ function commitHookEffectListMount(flags: HookFlags, finishedWork: Fiber) {
// Mount
const create = effect.create;
- effect.destroy = create();
+ if (__DEV__ && (flags & HookInsertion) !== NoHookEffect) {
+ setIsRunningInsertionEffect(true);
+ try {
+ effect.destroy = create();
+ } finally {
+ setIsRunningInsertionEffect(false);
+ }
+ } else {
+ effect.destroy = create();
+ }
if (enableSchedulingProfiler) {
if ((flags & HookPassive) !== NoHookEffect) {
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
index e962039d9ca9c..187a7776c8ada 100644
--- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js
+++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
@@ -136,6 +136,7 @@ import {
restorePendingUpdaters,
addTransitionStartCallbackToPendingTransition,
addTransitionCompleteCallbackToPendingTransition,
+ setIsRunningInsertionEffect,
} from './ReactFiberWorkLoop.old';
import {
NoFlags as NoHookEffect,
@@ -536,7 +537,16 @@ function commitHookEffectListUnmount(
}
}
- safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy);
+ if (__DEV__ && (flags & HookInsertion) !== NoHookEffect) {
+ setIsRunningInsertionEffect(true);
+ try {
+ safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy);
+ } finally {
+ setIsRunningInsertionEffect(false);
+ }
+ } else {
+ safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy);
+ }
if (enableSchedulingProfiler) {
if ((flags & HookPassive) !== NoHookEffect) {
@@ -570,7 +580,16 @@ function commitHookEffectListMount(flags: HookFlags, finishedWork: Fiber) {
// Mount
const create = effect.create;
- effect.destroy = create();
+ if (__DEV__ && (flags & HookInsertion) !== NoHookEffect) {
+ setIsRunningInsertionEffect(true);
+ try {
+ effect.destroy = create();
+ } finally {
+ setIsRunningInsertionEffect(false);
+ }
+ } else {
+ effect.destroy = create();
+ }
if (enableSchedulingProfiler) {
if ((flags & HookPassive) !== NoHookEffect) {
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
index 4da17c7bb6e80..c03c8a14cb004 100644
--- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -406,6 +406,8 @@ let nestedPassiveUpdateCount: number = 0;
let currentEventTime: number = NoTimestamp;
let currentEventTransitionLane: Lanes = NoLanes;
+let isRunningInsertionEffect = false;
+
export function getWorkInProgressRoot(): FiberRoot | null {
return workInProgressRoot;
}
@@ -517,6 +519,12 @@ export function scheduleUpdateOnFiber(
): FiberRoot | null {
checkForNestedUpdates();
+ if (__DEV__) {
+ if (isRunningInsertionEffect) {
+ console.error('useInsertionEffect must not schedule updates.');
+ }
+ }
+
const root = markUpdateLaneFromFiberToRoot(fiber, lane);
if (root === null) {
return null;
@@ -3132,3 +3140,9 @@ function warnIfSuspenseResolutionNotWrappedWithActDEV(root: FiberRoot): void {
}
}
}
+
+export function setIsRunningInsertionEffect(isRunning: boolean): void {
+ if (__DEV__) {
+ isRunningInsertionEffect = isRunning;
+ }
+}
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
index 2c940a71001db..926818d38b320 100644
--- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -406,6 +406,8 @@ let nestedPassiveUpdateCount: number = 0;
let currentEventTime: number = NoTimestamp;
let currentEventTransitionLane: Lanes = NoLanes;
+let isRunningInsertionEffect = false;
+
export function getWorkInProgressRoot(): FiberRoot | null {
return workInProgressRoot;
}
@@ -517,6 +519,12 @@ export function scheduleUpdateOnFiber(
): FiberRoot | null {
checkForNestedUpdates();
+ if (__DEV__) {
+ if (isRunningInsertionEffect) {
+ console.error('useInsertionEffect must not schedule updates.');
+ }
+ }
+
const root = markUpdateLaneFromFiberToRoot(fiber, lane);
if (root === null) {
return null;
@@ -3132,3 +3140,9 @@ function warnIfSuspenseResolutionNotWrappedWithActDEV(root: FiberRoot): void {
}
}
}
+
+export function setIsRunningInsertionEffect(isRunning: boolean): void {
+ if (__DEV__) {
+ isRunningInsertionEffect = isRunning;
+ }
+}
diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
index 811be09639158..dae042d6c6ed3 100644
--- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -3090,6 +3090,43 @@ describe('ReactHooksWithNoopRenderer', () => {
}),
).toThrow('is not a function');
});
+
+ it('warns when setState is called from insertion effect setup', () => {
+ function App(props) {
+ const [, setX] = useState(0);
+ useInsertionEffect(() => {
+ setX(1);
+ }, []);
+ return null;
+ }
+
+ const root = ReactNoop.createRoot();
+ expect(() =>
+ act(() => {
+ root.render(<App />);
+ }),
+ ).toErrorDev(['Warning: useInsertionEffect must not schedule updates.']);
+ });
+
+ it('warns when setState is called from insertion effect cleanup', () => {
+ function App(props) {
+ const [, setX] = useState(0);
+ useInsertionEffect(() => {
+ return () => setX(1);
+ }, [props.foo]);
+ return null;
+ }
+
+ const root = ReactNoop.createRoot();
+ act(() => {
+ root.render(<App foo="hello" />);
+ });
+ expect(() => {
+ act(() => {
+ root.render(<App foo="goodbye" />);
+ });
+ }).toErrorDev(['Warning: useInsertionEffect must not schedule updates.']);
+ });
});
describe('useLayoutEffect', () => {
From 59266f5c2c83c9756841333604b088dc0f2c6523 Mon Sep 17 00:00:00 2001
From: Dan Abramov <dan.abramov@gmail.com>
Date: Thu, 7 Apr 2022 20:04:50 +0100
Subject: [PATCH 2/2] Use existing DEV reset mechanism
---
.../src/ReactFiberCommitWork.new.js | 30 +++++------
.../src/ReactFiberCommitWork.old.js | 30 +++++------
.../src/ReactFiberWorkLoop.new.js | 3 ++
.../src/ReactFiberWorkLoop.old.js | 3 ++
.../ReactHooksWithNoopRenderer-test.js | 50 +++++++++++++++++--
5 files changed, 85 insertions(+), 31 deletions(-)
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
index aad83fa42c8c2..d1bbdcdde156d 100644
--- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js
+++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js
@@ -537,15 +537,16 @@ function commitHookEffectListUnmount(
}
}
- if (__DEV__ && (flags & HookInsertion) !== NoHookEffect) {
- setIsRunningInsertionEffect(true);
- try {
- safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy);
- } finally {
+ if (__DEV__) {
+ if ((flags & HookInsertion) !== NoHookEffect) {
+ setIsRunningInsertionEffect(true);
+ }
+ }
+ safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy);
+ if (__DEV__) {
+ if ((flags & HookInsertion) !== NoHookEffect) {
setIsRunningInsertionEffect(false);
}
- } else {
- safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy);
}
if (enableSchedulingProfiler) {
@@ -580,15 +581,16 @@ function commitHookEffectListMount(flags: HookFlags, finishedWork: Fiber) {
// Mount
const create = effect.create;
- if (__DEV__ && (flags & HookInsertion) !== NoHookEffect) {
- setIsRunningInsertionEffect(true);
- try {
- effect.destroy = create();
- } finally {
+ if (__DEV__) {
+ if ((flags & HookInsertion) !== NoHookEffect) {
+ setIsRunningInsertionEffect(true);
+ }
+ }
+ effect.destroy = create();
+ if (__DEV__) {
+ if ((flags & HookInsertion) !== NoHookEffect) {
setIsRunningInsertionEffect(false);
}
- } else {
- effect.destroy = create();
}
if (enableSchedulingProfiler) {
diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
index 187a7776c8ada..bff8b2781310b 100644
--- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js
+++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js
@@ -537,15 +537,16 @@ function commitHookEffectListUnmount(
}
}
- if (__DEV__ && (flags & HookInsertion) !== NoHookEffect) {
- setIsRunningInsertionEffect(true);
- try {
- safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy);
- } finally {
+ if (__DEV__) {
+ if ((flags & HookInsertion) !== NoHookEffect) {
+ setIsRunningInsertionEffect(true);
+ }
+ }
+ safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy);
+ if (__DEV__) {
+ if ((flags & HookInsertion) !== NoHookEffect) {
setIsRunningInsertionEffect(false);
}
- } else {
- safelyCallDestroy(finishedWork, nearestMountedAncestor, destroy);
}
if (enableSchedulingProfiler) {
@@ -580,15 +581,16 @@ function commitHookEffectListMount(flags: HookFlags, finishedWork: Fiber) {
// Mount
const create = effect.create;
- if (__DEV__ && (flags & HookInsertion) !== NoHookEffect) {
- setIsRunningInsertionEffect(true);
- try {
- effect.destroy = create();
- } finally {
+ if (__DEV__) {
+ if ((flags & HookInsertion) !== NoHookEffect) {
+ setIsRunningInsertionEffect(true);
+ }
+ }
+ effect.destroy = create();
+ if (__DEV__) {
+ if ((flags & HookInsertion) !== NoHookEffect) {
setIsRunningInsertionEffect(false);
}
- } else {
- effect.destroy = create();
}
if (enableSchedulingProfiler) {
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
index c03c8a14cb004..163aec8e7689f 100644
--- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -2531,6 +2531,9 @@ export function captureCommitPhaseError(
nearestMountedAncestor: Fiber | null,
error: mixed,
) {
+ if (__DEV__) {
+ setIsRunningInsertionEffect(false);
+ }
if (sourceFiber.tag === HostRoot) {
// Error was thrown at the root. There is no parent, so the root
// itself should capture it.
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
index 926818d38b320..fe16c38c230d1 100644
--- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js
@@ -2531,6 +2531,9 @@ export function captureCommitPhaseError(
nearestMountedAncestor: Fiber | null,
error: mixed,
) {
+ if (__DEV__) {
+ setIsRunningInsertionEffect(false);
+ }
if (sourceFiber.tag === HostRoot) {
// Error was thrown at the root. There is no parent, so the root
// itself should capture it.
diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
index dae042d6c6ed3..bacf9da7029a7 100644
--- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -3096,7 +3096,10 @@ describe('ReactHooksWithNoopRenderer', () => {
const [, setX] = useState(0);
useInsertionEffect(() => {
setX(1);
- }, []);
+ if (props.throw) {
+ throw Error('No');
+ }
+ }, [props.throw]);
return null;
}
@@ -3106,14 +3109,37 @@ describe('ReactHooksWithNoopRenderer', () => {
root.render(<App />);
}),
).toErrorDev(['Warning: useInsertionEffect must not schedule updates.']);
+
+ expect(() => {
+ act(() => {
+ root.render(<App throw={true} />);
+ });
+ }).toThrow('No');
+
+ // Should not warn for regular effects after throw.
+ function NotInsertion() {
+ const [, setX] = useState(0);
+ useEffect(() => {
+ setX(1);
+ }, []);
+ return null;
+ }
+ act(() => {
+ root.render(<NotInsertion />);
+ });
});
it('warns when setState is called from insertion effect cleanup', () => {
function App(props) {
const [, setX] = useState(0);
useInsertionEffect(() => {
- return () => setX(1);
- }, [props.foo]);
+ if (props.throw) {
+ throw Error('No');
+ }
+ return () => {
+ setX(1);
+ };
+ }, [props.throw, props.foo]);
return null;
}
@@ -3126,6 +3152,24 @@ describe('ReactHooksWithNoopRenderer', () => {
root.render(<App foo="goodbye" />);
});
}).toErrorDev(['Warning: useInsertionEffect must not schedule updates.']);
+
+ expect(() => {
+ act(() => {
+ root.render(<App throw={true} />);
+ });
+ }).toThrow('No');
+
+ // Should not warn for regular effects after throw.
+ function NotInsertion() {
+ const [, setX] = useState(0);
+ useEffect(() => {
+ setX(1);
+ }, []);
+ return null;
+ }
+ act(() => {
+ root.render(<NotInsertion />);
+ });
});
});