fix: don't run effects if a render phase update results in unchanged deps (#20676)
The memoized state of effect hooks is only invalidated when deps change. Deps are compared between the previous effect and the current effect. This can be problematic if one commit consists of an update that has changed deps followed by an update that has equal deps. That commit will lead to memoizedState containing the changed deps even though we committed with unchanged deps. The n+1 update will therefore run an effect because we compare the updated deps with the deps with which we never actually committed. To prevent this we now invalidate memoizedState on every updateEffectImpl call so that memoizedStat.deps always points to the latest deps.
Sebastian Silbermann committed
Jan 29, 2021 at 16:51 UTC
bb1b7951d182d36345c83154f9d9bab218011b46
4 files changed
+90
-3
packages/react-devtools-shared/src/backend/renderer.js
+45
-1
@@ -103,6 +103,7 @@ import type {
103
ComponentFilter,
104
ElementType,
105
} from 'react-devtools-shared/src/types';
106
+import is from 'shared/objectIs';
107
108
type getDisplayNameForFiberType = (fiber: Fiber) => string | null;
109
type getTypeSymbolType = (type: any) => Symbol | number;
@@ -1073,6 +1074,49 @@ export function attach(
1074
return null;
1075
}
1076
1077
+ function areHookInputsEqual(
1078
+ nextDeps: Array<mixed>,
1079
+ prevDeps: Array<mixed> | null,
1080
+ ) {
1081
+ if (prevDeps === null) {
1082
+ return false;
1083
+ }
1084
+
1085
+ for (let i = 0; i < prevDeps.length && i < nextDeps.length; i++) {
1086
+ if (is(nextDeps[i], prevDeps[i])) {
1087
+ continue;
1088
+ }
1089
+ return false;
1090
+ }
1091
+ return true;
1092
+ }
1093
+
1094
+ function isEffect(memoizedState) {
1095
+ return (
1096
+ memoizedState !== null &&
1097
+ typeof memoizedState === 'object' &&
1098
+ memoizedState.hasOwnProperty('tag') &&
1099
+ memoizedState.hasOwnProperty('create') &&
1100
+ memoizedState.hasOwnProperty('destroy') &&
1101
+ memoizedState.hasOwnProperty('deps') &&
1102
+ (memoizedState.deps === null || Array.isArray(memoizedState.deps)) &&
1103
+ memoizedState.hasOwnProperty('next')
1104
+ );
1105
+ }
1106
+
1107
+ function didHookChange(prev: any, next: any): boolean {
1108
+ const prevMemoizedState = prev.memoizedState;
1109
+ const nextMemoizedState = next.memoizedState;
1110
+
1111
+ if (isEffect(prevMemoizedState) && isEffect(nextMemoizedState)) {
1112
+ return (
1113
+ prevMemoizedState !== nextMemoizedState &&
1114
+ !areHookInputsEqual(nextMemoizedState.deps, prevMemoizedState.deps)
1115
+ );
1116
+ }
1117
+ return nextMemoizedState !== prevMemoizedState;
1118
+ }
1119
+
1120
function didHooksChange(prev: any, next: any): boolean {
1121
if (prev == null || next == null) {
1122
return false;
@@ -1086,7 +1130,7 @@ export function attach(
1130
next.hasOwnProperty('queue')
1131
) {
1132
while (next !== null) {
1089
- if (next.memoizedState !== prev.memoizedState) {
1133
+ if (didHookChange(prev, next)) {
1134
return true;
1135
} else {
1136
next = next.next;
packages/react-reconciler/src/ReactFiberHooks.new.js
+1
-1
@@ -1365,7 +1365,7 @@ function updateEffectImpl(fiberFlags, hookFlags, create, deps): void {
1365
if (nextDeps !== null) {
1366
const prevDeps = prevEffect.deps;
1367
if (areHookInputsEqual(nextDeps, prevDeps)) {
1368
- pushEffect(hookFlags, create, destroy, nextDeps);
1368
+ hook.memoizedState = pushEffect(hookFlags, create, destroy, nextDeps);
1369
return;
1370
}
1371
}
packages/react-reconciler/src/ReactFiberHooks.old.js
+1
-1
@@ -1344,7 +1344,7 @@ function updateEffectImpl(fiberFlags, hookFlags, create, deps): void {
1344
if (nextDeps !== null) {
1345
const prevDeps = prevEffect.deps;
1346
if (areHookInputsEqual(nextDeps, prevDeps)) {
1347
- pushEffect(hookFlags, create, destroy, nextDeps);
1347
+ hook.memoizedState = pushEffect(hookFlags, create, destroy, nextDeps);
1348
return;
1349
}
1350
}
packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+43
@@ -3760,4 +3760,47 @@ describe('ReactHooksWithNoopRenderer', () => {
3760
// effects would not fire.
3761
expect(Scheduler).toHaveYielded(['Unmount A', 'Unmount B']);
3762
});
3763
+
3764
+ it('effect dependencies are persisted after a render phase update', () => {
3765
+ let handleClick;
3766
+ function Test() {
3767
+ const [count, setCount] = useState(0);
3768
+
3769
+ useEffect(() => {
3770
+ Scheduler.unstable_yieldValue(`Effect: ${count}`);
3771
+ }, [count]);
3772
+
3773
+ if (count > 0) {
3774
+ setCount(0);
3775
+ }
3776
+
3777
+ handleClick = () => setCount(2);
3778
+
3779
+ return <Text text={`Render: ${count}`} />;
3780
+ }
3781
+
3782
+ ReactNoop.act(() => {
3783
+ ReactNoop.render(<Test />);
3784
+ });
3785
+
3786
+ expect(Scheduler).toHaveYielded(['Render: 0', 'Effect: 0']);
3787
+
3788
+ ReactNoop.act(() => {
3789
+ handleClick();
3790
+ });
3791
+
3792
+ expect(Scheduler).toHaveYielded(['Render: 0']);
3793
+
3794
+ ReactNoop.act(() => {
3795
+ handleClick();
3796
+ });
3797
+
3798
+ expect(Scheduler).toHaveYielded(['Render: 0']);
3799
+
3800
+ ReactNoop.act(() => {
3801
+ handleClick();
3802
+ });
3803
+
3804
+ expect(Scheduler).toHaveYielded(['Render: 0']);
3805
+ });
3806
});