Bugfix: Effect clean up when deleting suspended tree (#19752)
* Bug: Effect clean up when deleting suspended tree Adds a failing unit test. * Re-use static flags from suspended primary tree When switching to a Suspense boundary's fallback, we need to be sure to preserve static subtree flags from the primary tree.
Andrew Clark committed
Sep 4, 2020 at 17:46 UTC
4f3f7eeb7f467be7a8c5e7c2cdf64cb95a6099c3
2 files changed
+104
packages/react-reconciler/src/ReactFiberBeginWork.new.js
+7
@@ -64,6 +64,7 @@ import {
64
Ref,
65
Deletion,
66
ForceUpdateForLegacySuspense,
67
+ StaticMask,
68
} from './ReactFiberFlags';
69
import ReactSharedInternals from 'shared/ReactSharedInternals';
70
import {
@@ -2131,6 +2132,12 @@ function updateSuspenseFallbackChildren(
2132
currentPrimaryChildFragment,
2133
primaryChildProps,
2134
);
2135
+
2136
+ // Since we're reusing a current tree, we need to reuse the flags, too.
2137
+ // (We don't do this in legacy mode, because in legacy mode we don't re-use
2138
+ // the current tree; see previous branch.)
2139
+ primaryChildFragment.subtreeFlags =
2140
+ currentPrimaryChildFragment.subtreeFlags & StaticMask;
2141
}
2142
let fallbackChildFragment;
2143
if (currentFallbackChildFragment !== null) {
packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
+97
@@ -3956,4 +3956,101 @@ describe('ReactSuspenseWithNoopRenderer', () => {
3956
</>,
3957
);
3958
});
3959
+
3960
+ it('should fire effect clean-up when deleting suspended tree', async () => {
3961
+ const {useEffect} = React;
3962
+
3963
+ function App({show}) {
3964
+ return (
3965
+ <Suspense fallback={<Text text="Loading..." />}>
3966
+ <Child />
3967
+ {show && <AsyncText text="Async" />}
3968
+ </Suspense>
3969
+ );
3970
+ }
3971
+
3972
+ function Child() {
3973
+ useEffect(() => {
3974
+ Scheduler.unstable_yieldValue('Mount Child');
3975
+ return () => {
3976
+ Scheduler.unstable_yieldValue('Unmount Child');
3977
+ };
3978
+ }, []);
3979
+ return <span prop="Child" />;
3980
+ }
3981
+
3982
+ const root = ReactNoop.createRoot();
3983
+
3984
+ await ReactNoop.act(async () => {
3985
+ root.render(<App show={false} />);
3986
+ });
3987
+ expect(Scheduler).toHaveYielded(['Mount Child']);
3988
+ expect(root).toMatchRenderedOutput(<span prop="Child" />);
3989
+
3990
+ await ReactNoop.act(async () => {
3991
+ root.render(<App show={true} />);
3992
+ });
3993
+ // TODO: `act` should have already flushed the placeholder, so this
3994
+ // runAllTimers call should be unnecessary.
3995
+ jest.runAllTimers();
3996
+ expect(Scheduler).toHaveYielded(['Suspend! [Async]', 'Loading...']);
3997
+ expect(root).toMatchRenderedOutput(
3998
+ <>
3999
+ <span hidden={true} prop="Child" />
4000
+ <span prop="Loading..." />
4001
+ </>,
4002
+ );
4003
+
4004
+ await ReactNoop.act(async () => {
4005
+ root.render(null);
4006
+ });
4007
+ expect(Scheduler).toHaveYielded(['Unmount Child']);
4008
+ });
4009
+
4010
+ it('should fire effect clean-up when deleting suspended tree (legacy)', async () => {
4011
+ const {useEffect} = React;
4012
+
4013
+ function App({show}) {
4014
+ return (
4015
+ <Suspense fallback={<Text text="Loading..." />}>
4016
+ <Child />
4017
+ {show && <AsyncText text="Async" />}
4018
+ </Suspense>
4019
+ );
4020
+ }
4021
+
4022
+ function Child() {
4023
+ useEffect(() => {
4024
+ Scheduler.unstable_yieldValue('Mount Child');
4025
+ return () => {
4026
+ Scheduler.unstable_yieldValue('Unmount Child');
4027
+ };
4028
+ }, []);
4029
+ return <span prop="Child" />;
4030
+ }
4031
+
4032
+ const root = ReactNoop.createLegacyRoot();
4033
+
4034
+ await ReactNoop.act(async () => {
4035
+ root.render(<App show={false} />);
4036
+ });
4037
+ expect(Scheduler).toHaveYielded(['Mount Child']);
4038
+ expect(root).toMatchRenderedOutput(<span prop="Child" />);
4039
+
4040
+ await ReactNoop.act(async () => {
4041
+ root.render(<App show={true} />);
4042
+ });
4043
+ expect(Scheduler).toHaveYielded(['Suspend! [Async]', 'Loading...']);
4044
+ expect(root).toMatchRenderedOutput(
4045
+ <>
4046
+ <span hidden={true} prop="Child" />
4047
+ <span prop="Loading..." />
4048
+ </>,
4049
+ );
4050
+
4051
+ await ReactNoop.act(async () => {
4052
+ root.render(null);
4053
+ });
4054
+ expect(Scheduler).toHaveYielded(['Unmount Child']);
4055
+ });
4056
});