Skip to content

Commit

Permalink
fix: don't run effects if a render phase update results in unchanged …
Browse files Browse the repository at this point in the history
…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.
  • Loading branch information
eps1lon authored Jan 29, 2021
1 parent 766a7a2 commit bb1b795
Show file tree
Hide file tree
Showing 4 changed files with 90 additions and 3 deletions.
46 changes: 45 additions & 1 deletion packages/react-devtools-shared/src/backend/renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -1073,6 +1074,49 @@ export function attach(
return null;
}

function areHookInputsEqual(
nextDeps: Array<mixed>,
prevDeps: Array<mixed> | 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;
Expand All @@ -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;
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberHooks.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/react-reconciler/src/ReactFiberHooks.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 <Text text={`Render: ${count}`} />;
}

ReactNoop.act(() => {
ReactNoop.render(<Test />);
});

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']);
});
});

0 comments on commit bb1b795

Please sign in to comment.