From c8435e702cdcd99aeb2abebb76f67b6380da8a7e Mon Sep 17 00:00:00 2001 From: eps1lon Date: Wed, 27 Jan 2021 14:00:16 +0100 Subject: [PATCH 1/9] current behavior of effect dependencies on render phase updates --- .../ReactHooksWithNoopRenderer-test.js | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index a792f8609be44..8128a33534c82 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 consistent with yielded values when triggering state updates during render', () => { + 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', 'Effect: 0']); + + ReactNoop.act(() => { + handleClick(); + }); + + expect(Scheduler).toHaveYielded(['Render: 0']); + }); }); From 5bcce8353108c983019f47ac9575398050211849 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Wed, 27 Jan 2021 14:00:47 +0100 Subject: [PATCH 2/9] fix: don't schedule effects when render phase updates aren't committed --- packages/react-reconciler/src/ReactFiberHooks.old.js | 2 +- .../src/__tests__/ReactHooksWithNoopRenderer-test.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js index 26721dcd11264..190e12f11dec1 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.old.js +++ b/packages/react-reconciler/src/ReactFiberHooks.old.js @@ -1305,7 +1305,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 8128a33534c82..2daa3bca10569 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -3795,7 +3795,7 @@ describe('ReactHooksWithNoopRenderer', () => { handleClick(); }); - expect(Scheduler).toHaveYielded(['Render: 0', 'Effect: 0']); + expect(Scheduler).toHaveYielded(['Render: 0']); ReactNoop.act(() => { handleClick(); From 6cfd74d65febffd304131d699f04f1779180a59f Mon Sep 17 00:00:00 2001 From: eps1lon Date: Wed, 27 Jan 2021 14:09:46 +0100 Subject: [PATCH 3/9] apply fix to new fork --- packages/react-reconciler/src/ReactFiberHooks.new.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js index 374d383030cdb..8fef31672f106 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.new.js +++ b/packages/react-reconciler/src/ReactFiberHooks.new.js @@ -1328,7 +1328,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; } } From 9dfcd56044694b0acd3bb6dfab17a0fba6984d84 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Wed, 27 Jan 2021 21:22:48 +0100 Subject: [PATCH 4/9] Use suggested test name --- .../src/__tests__/ReactHooksWithNoopRenderer-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index 2daa3bca10569..cc86edb132209 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -3761,7 +3761,7 @@ describe('ReactHooksWithNoopRenderer', () => { expect(Scheduler).toHaveYielded(['Unmount A', 'Unmount B']); }); - it('effect dependencies are consistent with yielded values when triggering state updates during render', () => { + it('effect dependencies are persisted after a render phase update', () => { let handleClick; function Test() { const [count, setCount] = useState(0); From 0f58f40083a8a0804792eee706c0f04ec91194d7 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Wed, 27 Jan 2021 22:01:01 +0100 Subject: [PATCH 5/9] devtools: compare effects by deps not referential equality --- .../src/backend/renderer.js | 45 ++++++++++++++++++- 1 file changed, 44 insertions(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index cc82fda74cdb1..75f7c78848a22 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,48 @@ 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.hasOwnProperty('next') + ); + } + + function didHookChange(prev: Hook, next: Hook): boolean { + const prevMemoizedState = prev.memoizedState; + const nextMemoizedState = next.memoizedState; + + if (isEffect(prevMemoizedState) && isEffect(nextMemoizedState)) { + return !areHookInputsEqual( + nextMemoizedState.deps, + prevMemoizedState.deps, + ); + } + return nextMemoizedState !== prevMemoizedState; + } + function didHooksChange(prev: any, next: any): boolean { if (prev == null || next == null) { return false; @@ -1086,7 +1129,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; From 3cdd353b7f64f6b078e7e33be06dacc0935b5b57 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Wed, 27 Jan 2021 22:08:59 +0100 Subject: [PATCH 6/9] disable type checking for added functions --- packages/react-devtools-shared/src/backend/renderer.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 75f7c78848a22..6ee4bd0480459 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -1103,7 +1103,7 @@ export function attach( ); } - function didHookChange(prev: Hook, next: Hook): boolean { + function didHookChange(prev: any, next: any): boolean { const prevMemoizedState = prev.memoizedState; const nextMemoizedState = next.memoizedState; From e39a670f3557d0ba516d951952dc1f0636d02070 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Wed, 27 Jan 2021 22:18:20 +0100 Subject: [PATCH 7/9] bump CircleCI From 77ed7276d08428617a384f50ee9547b2dca246dd Mon Sep 17 00:00:00 2001 From: eps1lon Date: Thu, 28 Jan 2021 12:07:01 +0100 Subject: [PATCH 8/9] Be more defensive about `useState({ deps })` --- packages/react-devtools-shared/src/backend/renderer.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index 6ee4bd0480459..b9c619e089a14 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -1099,6 +1099,7 @@ export function attach( memoizedState.hasOwnProperty('create') && memoizedState.hasOwnProperty('destroy') && memoizedState.hasOwnProperty('deps') && + (memoizedState.deps === null || Array.isArray(memoizedState.deps)) && memoizedState.hasOwnProperty('next') ); } From 5c24bb5fc195458719b943fdeb885d4bd29b85af Mon Sep 17 00:00:00 2001 From: eps1lon Date: Fri, 29 Jan 2021 08:23:58 +0100 Subject: [PATCH 9/9] bail out early if memoizedState didn't change --- packages/react-devtools-shared/src/backend/renderer.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index b9c619e089a14..a9e3b2aa0b415 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -1109,9 +1109,9 @@ export function attach( const nextMemoizedState = next.memoizedState; if (isEffect(prevMemoizedState) && isEffect(nextMemoizedState)) { - return !areHookInputsEqual( - nextMemoizedState.deps, - prevMemoizedState.deps, + return ( + prevMemoizedState !== nextMemoizedState && + !areHookInputsEqual(nextMemoizedState.deps, prevMemoizedState.deps) ); } return nextMemoizedState !== prevMemoizedState;