Skip to content

Commit

Permalink
Add failing test for passive effect cleanup functions and memoized co…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
Brian Vaughn authored Sep 2, 2020
1 parent 2cfd73c commit 99cae88
Show file tree
Hide file tree
Showing 2 changed files with 156 additions and 0 deletions.
18 changes: 18 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand All @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down

0 comments on commit 99cae88

Please sign in to comment.