From bb1b7951d182d36345c83154f9d9bab218011b46 Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Fri, 29 Jan 2021 16:51:11 +0100 Subject: [PATCH] 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. --- .../src/backend/renderer.js | 46 ++++++++++++++++++- .../src/ReactFiberHooks.new.js | 2 +- .../src/ReactFiberHooks.old.js | 2 +- .../ReactHooksWithNoopRenderer-test.js | 43 +++++++++++++++++ 4 files changed, 90 insertions(+), 3 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index cc82fda74cdb1..a9e3b2aa0b415 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -103,6 +103,7 @@ import type { ComponentFilter, ElementType, } from 'react-devtools-shared/src/types'; +import is from 'shared/objectIs'; type getDisplayNameForFiberType = (fiber: Fiber) => string | null; type getTypeSymbolType = (type: any) => Symbol | number; @@ -1073,6 +1074,49 @@ export function attach( return null; } + function areHookInputsEqual( + nextDeps: Array, + prevDeps: Array | null, + ) { + if (prevDeps === null) { + return false; + } + + for (let i = 0; i < prevDeps.length && i < nextDeps.length; i++) { + if (is(nextDeps[i], prevDeps[i])) { + continue; + } + return false; + } + return true; + } + + function isEffect(memoizedState) { + return ( + memoizedState !== null && + typeof memoizedState === 'object' && + memoizedState.hasOwnProperty('tag') && + memoizedState.hasOwnProperty('create') && + memoizedState.hasOwnProperty('destroy') && + memoizedState.hasOwnProperty('deps') && + (memoizedState.deps === null || Array.isArray(memoizedState.deps)) && + memoizedState.hasOwnProperty('next') + ); + } + + function didHookChange(prev: any, next: any): boolean { + const prevMemoizedState = prev.memoizedState; + const nextMemoizedState = next.memoizedState; + + if (isEffect(prevMemoizedState) && isEffect(nextMemoizedState)) { + return ( + prevMemoizedState !== nextMemoizedState && + !areHookInputsEqual(nextMemoizedState.deps, prevMemoizedState.deps) + ); + } + return nextMemoizedState !== prevMemoizedState; + } + function didHooksChange(prev: any, next: any): boolean { if (prev == null || next == null) { return false; @@ -1086,7 +1130,7 @@ export function attach( next.hasOwnProperty('queue') ) { while (next !== null) { - if (next.memoizedState !== prev.memoizedState) { + if (didHookChange(prev, next)) { return true; } else { next = next.next; diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index bac6749f64465..fe49e725fd7fc 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -1365,7 +1365,7 @@ function updateEffectImpl(fiberFlags, hookFlags, create, deps): void { if (nextDeps !== null) { const prevDeps = prevEffect.deps; if (areHookInputsEqual(nextDeps, prevDeps)) { - pushEffect(hookFlags, create, destroy, nextDeps); + hook.memoizedState = pushEffect(hookFlags, create, destroy, nextDeps); return; } } diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index 6a42f73527f93..ccb719dc3d8a1 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -1344,7 +1344,7 @@ function updateEffectImpl(fiberFlags, hookFlags, create, deps): void { if (nextDeps !== null) { const prevDeps = prevEffect.deps; if (areHookInputsEqual(nextDeps, prevDeps)) { - pushEffect(hookFlags, create, destroy, nextDeps); + hook.memoizedState = pushEffect(hookFlags, create, destroy, nextDeps); return; } } diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index a792f8609be44..cc86edb132209 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -3760,4 +3760,47 @@ describe('ReactHooksWithNoopRenderer', () => { // effects would not fire. expect(Scheduler).toHaveYielded(['Unmount A', 'Unmount B']); }); + + it('effect dependencies are persisted after a render phase update', () => { + let handleClick; + function Test() { + const [count, setCount] = useState(0); + + useEffect(() => { + Scheduler.unstable_yieldValue(`Effect: ${count}`); + }, [count]); + + if (count > 0) { + setCount(0); + } + + handleClick = () => setCount(2); + + return ; + } + + ReactNoop.act(() => { + ReactNoop.render(); + }); + + expect(Scheduler).toHaveYielded(['Render: 0', 'Effect: 0']); + + ReactNoop.act(() => { + handleClick(); + }); + + expect(Scheduler).toHaveYielded(['Render: 0']); + + ReactNoop.act(() => { + handleClick(); + }); + + expect(Scheduler).toHaveYielded(['Render: 0']); + + ReactNoop.act(() => { + handleClick(); + }); + + expect(Scheduler).toHaveYielded(['Render: 0']); + }); });