fix: don't run effects if a render phase update results in unchanged deps (#20676)

eps1lon · web-flow · commit bb1b7951d182 · 2021-01-29T10:51:11.000-05:00
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.
diff --git 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<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;
@@ -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
@@ -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
@@ -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
@@ -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']);
+  });
 });
