Add failing test for passive effect cleanup functions and memoized co…

…mponents (#19750) * Add failing tests for passive effects cleanup not being called for memoized components * Bubble passive static subtreeTag even after bailout This prevents subsequent unmounts from skipping over any pending passive effect destroy functions
facebook · Sep 2, 2020 · 99cae88 · 99cae88
1 parent 2cfd73c
commit 99cae88
Show file tree

Hide file tree

Showing 2 changed files with 156 additions and 0 deletions.
diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
@@ -1904,6 +1904,14 @@ function resetChildLanes(completedWork: Fiber) {
           mergeLanes(child.lanes, child.childLanes),
         );
 
+        // Preserve passive static flag even in the case of a bailout;
+        // otherwise a subsequent unmount may bailout before calling destroy functions.
+        subtreeTag |= child.subtreeTag & PassiveStaticSubtreeTag;
+        const effectTag = child.effectTag;
+        if ((effectTag & PassiveStatic) !== NoEffect) {
+          subtreeTag |= PassiveStaticSubtreeTag;
+        }
+
         treeBaseDuration += child.treeBaseDuration;
         child = child.sibling;
       }
@@ -1928,9 +1936,19 @@ function resetChildLanes(completedWork: Fiber) {
           mergeLanes(child.lanes, child.childLanes),
         );
 
+        // Preserve passive static flag even in the case of a bailout;
+        // otherwise a subsequent unmount may bailout before calling destroy functions.
+        subtreeTag |= child.subtreeTag & PassiveStaticSubtreeTag;
+        const effectTag = child.effectTag;
+        if ((effectTag & PassiveStatic) !== NoEffect) {
+          subtreeTag |= PassiveStaticSubtreeTag;
+        }
+
         child = child.sibling;
       }
     }
+
+    completedWork.subtreeTag |= subtreeTag;
   }
 
   completedWork.childLanes = newChildLanes;

diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -2734,6 +2734,144 @@ describe('ReactHooksWithNoopRenderer', () => {
         expect(ReactNoop.getChildren()).toEqual([]);
       });
     });
+
+    it('calls passive effect destroy functions for memoized components', () => {
+      const Wrapper = ({children}) => children;
+      function Child() {
+        React.useEffect(() => {
+          Scheduler.unstable_yieldValue('passive create');
+          return () => {
+            Scheduler.unstable_yieldValue('passive destroy');
+          };
+        }, []);
+        React.useLayoutEffect(() => {
+          Scheduler.unstable_yieldValue('layout create');
+          return () => {
+            Scheduler.unstable_yieldValue('layout destroy');
+          };
+        }, []);
+        Scheduler.unstable_yieldValue('render');
+        return null;
+      }
+
+      const isEqual = (prevProps, nextProps) =>
+        prevProps.prop === nextProps.prop;
+      const MemoizedChild = React.memo(Child, isEqual);
+
+      act(() => {
+        ReactNoop.render(
+          <Wrapper>
+            <MemoizedChild key={1} />
+          </Wrapper>,
+        );
+      });
+      expect(Scheduler).toHaveYielded([
+        'render',
+        'layout create',
+        'passive create',
+      ]);
+
+      // Include at least one no-op (memoized) update to trigger original bug.
+      act(() => {
+        ReactNoop.render(
+          <Wrapper>
+            <MemoizedChild key={1} />
+          </Wrapper>,
+        );
+      });
+      expect(Scheduler).toHaveYielded([]);
+
+      act(() => {
+        ReactNoop.render(
+          <Wrapper>
+            <MemoizedChild key={2} />
+          </Wrapper>,
+        );
+      });
+      expect(Scheduler).toHaveYielded([
+        'render',
+        'layout destroy',
+        'layout create',
+        'passive destroy',
+        'passive create',
+      ]);
+
+      act(() => {
+        ReactNoop.render(null);
+      });
+      expect(Scheduler).toHaveYielded(['layout destroy', 'passive destroy']);
+    });
+
+    it('calls passive effect destroy functions for descendants of memoized components', () => {
+      const Wrapper = ({children}) => children;
+      function Child() {
+        return <Grandchild />;
+      }
+
+      function Grandchild() {
+        React.useEffect(() => {
+          Scheduler.unstable_yieldValue('passive create');
+          return () => {
+            Scheduler.unstable_yieldValue('passive destroy');
+          };
+        }, []);
+        React.useLayoutEffect(() => {
+          Scheduler.unstable_yieldValue('layout create');
+          return () => {
+            Scheduler.unstable_yieldValue('layout destroy');
+          };
+        }, []);
+        Scheduler.unstable_yieldValue('render');
+        return null;
+      }
+
+      const isEqual = (prevProps, nextProps) =>
+        prevProps.prop === nextProps.prop;
+      const MemoizedChild = React.memo(Child, isEqual);
+
+      act(() => {
+        ReactNoop.render(
+          <Wrapper>
+            <MemoizedChild key={1} />
+          </Wrapper>,
+        );
+      });
+      expect(Scheduler).toHaveYielded([
+        'render',
+        'layout create',
+        'passive create',
+      ]);
+
+      // Include at least one no-op (memoized) update to trigger original bug.
+      act(() => {
+        ReactNoop.render(
+          <Wrapper>
+            <MemoizedChild key={1} />
+          </Wrapper>,
+        );
+      });
+      expect(Scheduler).toHaveYielded([]);
+
+      act(() => {
+        ReactNoop.render(
+          <Wrapper>
+            <MemoizedChild key={2} />
+          </Wrapper>,
+        );
+      });
+      expect(Scheduler).toHaveYielded([
+        'render',
+        'layout destroy',
+        'layout create',
+        'passive destroy',
+        'passive create',
+      ]);
+
+      act(() => {
+        ReactNoop.render(null);
+      });
+      expect(Scheduler).toHaveYielded(['layout destroy', 'passive destroy']);
+    });
   });
 
   describe('useLayoutEffect', () => {