@samitouri / QOS-React / commits / e27720d7f5

[Synchronous Suspense] Reuse deletions from primary tree (#14133)

Fixes a bug where deletion effects in the primary tree were dropped before entering the second render pass. Because we no longer reset the effect list after the first render pass, I've also moved the deletion of the fallback children to the complete phase, after the tree successfully renders without suspending. Will need to revisit this heuristic when we implement resuming.

Andrew Clark committed Nov 7, 2018 at 10:56 UTC e27720d7f586cdadb00b0de485e41ec4dbcebe52

4 files changed +88 -68

packages/react-reconciler/src/ReactFiberBeginWork.js

+10 -66

@@ -1127,11 +1127,7 @@ function updateSuspenseComponent(

1127 progressedState !== null

1128 ? (workInProgress.child: any).child

1129 : (workInProgress.child: any);

1130 - reuseProgressedPrimaryChild(

1131 - workInProgress,

1132 - primaryChildFragment,

1133 - progressedPrimaryChild,

1134 - );

1130 + primaryChildFragment.child = progressedPrimaryChild;

1131 }

1132

1133 const fallbackChildFragment = createFiberFromFragment(

@@ -1186,11 +1182,7 @@ function updateSuspenseComponent(

1182 ? (workInProgress.child: any).child

1183 : (workInProgress.child: any);

1184 if (progressedPrimaryChild !== currentPrimaryChildFragment.child) {

1189 - reuseProgressedPrimaryChild(

1190 - workInProgress,

1191 - primaryChildFragment,

1192 - progressedPrimaryChild,

1193 - );

1185 + primaryChildFragment.child = progressedPrimaryChild;

1186 }

1187 }

1188

@@ -1213,20 +1205,19 @@ function updateSuspenseComponent(

1205 // and remove the intermediate fragment fiber.

1206 const nextPrimaryChildren = nextProps.children;

1207 const currentPrimaryChild = currentPrimaryChildFragment.child;

1216 - const currentFallbackChild = currentFallbackChildFragment.child;

1208 const primaryChild = reconcileChildFibers(

1209 workInProgress,

1210 currentPrimaryChild,

1211 nextPrimaryChildren,

1212 renderExpirationTime,

1213 );

1223 - // Delete the fallback children.

1224 - reconcileChildFibers(

1225 - workInProgress,

1226 - currentFallbackChild,

1227 - null,

1228 - renderExpirationTime,

1229 - );

1214 +

1215 + // If this render doesn't suspend, we need to delete the fallback

1216 + // children. Wait until the complete phase, after we've confirmed the

1217 + // fallback is no longer needed.

1218 + // TODO: Would it be better to store the fallback fragment on

1219 + // the stateNode?

1220 +

1221 // Continue rendering the children, like we normally do.

1222 child = next = primaryChild;

1223 }

@@ -1258,11 +1249,7 @@ function updateSuspenseComponent(

1249 progressedState !== null

1250 ? (workInProgress.child: any).child

1251 : (workInProgress.child: any);

1261 - reuseProgressedPrimaryChild(

1262 - workInProgress,

1263 - primaryChildFragment,

1264 - progressedPrimaryChild,

1265 - );

1252 + primaryChildFragment.child = progressedPrimaryChild;

1253 }

1254

1255 // Create a fragment from the fallback children, too.

@@ -1298,49 +1285,6 @@ function updateSuspenseComponent(

1285 return next;

1286 }

1287

1301 -function reuseProgressedPrimaryChild(

1302 - workInProgress: Fiber,

1303 - primaryChildFragment: Fiber,

1304 - progressedChild: Fiber | null,

1305 -) {

1306 - // This function is only called outside concurrent mode. Usually, if a work-

1307 - // in-progress primary tree suspends, we throw it out and revert back to

1308 - // current. Outside concurrent mode, though, we commit the suspended work-in-

1309 - // progress, even though it didn't complete. This function reuses the children

1310 - // and transfers the effects.

1311 - let child = (primaryChildFragment.child = progressedChild);

1312 - while (child !== null) {

1313 - // Ensure that the first and last effect of the parent corresponds

1314 - // to the children's first and last effect.

1315 - if (primaryChildFragment.firstEffect === null) {

1316 - primaryChildFragment.firstEffect = child.firstEffect;

1317 - }

1318 - if (child.lastEffect !== null) {

1319 - if (primaryChildFragment.lastEffect !== null) {

1320 - primaryChildFragment.lastEffect.nextEffect = child.firstEffect;

1321 - }

1322 - primaryChildFragment.lastEffect = child.lastEffect;

1323 - }

1324 -

1325 - // Append all the effects of the subtree and this fiber onto the effect

1326 - // list of the parent. The completion order of the children affects the

1327 - // side-effect order.

1328 - if (child.effectTag > PerformedWork) {

1329 - if (primaryChildFragment.lastEffect !== null) {

1330 - primaryChildFragment.lastEffect.nextEffect = child;

1331 - } else {

1332 - primaryChildFragment.firstEffect = child;

1333 - }

1334 - primaryChildFragment.lastEffect = child;

1335 - }

1336 - child.return = primaryChildFragment;

1337 - child = child.sibling;

1338 - }

1339 -

1340 - workInProgress.firstEffect = primaryChildFragment.firstEffect;

1341 - workInProgress.lastEffect = primaryChildFragment.lastEffect;

1342 -}

1343 -

1288 function updatePortalComponent(

1289 current: Fiber | null,

1290 workInProgress: Fiber,

packages/react-reconciler/src/ReactFiberCompleteWork.js

+19 -1

@@ -82,6 +82,7 @@ import {

82 popHydrationState,

83 } from './ReactFiberHydrationContext';

84 import {ConcurrentMode, NoContext} from './ReactTypeOfMode';

85 +import {reconcileChildFibers} from './ReactChildFiber';

87 function markUpdate(workInProgress: Fiber) {

88 // Tag the fiber with an update effect. This turns a Placement into

@@ -700,12 +701,29 @@ function completeWork(

701 if ((workInProgress.effectTag & DidCapture) !== NoEffect) {

702 // Something suspended. Re-render with the fallback children.

703 workInProgress.expirationTime = renderExpirationTime;

703 - workInProgress.firstEffect = workInProgress.lastEffect = null;

704 + // Do not reset the effect list.

705 return workInProgress;

706 }

707

708 const nextDidTimeout = nextState !== null;

709 const prevDidTimeout = current !== null && current.memoizedState !== null;

710 +

711 + if (current !== null && !nextDidTimeout && prevDidTimeout) {

712 + // We just switched from the fallback to the normal children. Delete

713 + // the fallback.

714 + // TODO: Would it be better to store the fallback fragment on

715 + // the stateNode during the begin phase?

716 + const currentFallbackChild: Fiber | null = (current.child: any).sibling;

717 + if (currentFallbackChild !== null) {

718 + reconcileChildFibers(

719 + workInProgress,

720 + currentFallbackChild,

721 + null,

722 + renderExpirationTime,

723 + );

724 + }

725 + }

726 +

727 // The children either timed out after previously being visible, or

728 // were restored after previously being hidden. Schedule an effect

729 // to update their visiblity.

packages/react-reconciler/src/ReactFiberScheduler.js

-1

@@ -993,7 +993,6 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null {

993

994 if (nextUnitOfWork !== null) {

995 // Completing this fiber spawned new work. Work on that next.

996 - nextUnitOfWork.firstEffect = nextUnitOfWork.lastEffect = null;

996 return nextUnitOfWork;

997 }

998

packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js

+59

@@ -779,5 +779,64 @@ describe('ReactSuspense', () => {

779 expect(root).toFlushAndYield(['Child 1', 'Child 2']);

780 expect(root).toMatchRenderedOutput(['Child 1', 'Child 2'].join(''));

781 });

782 +

783 + it('reuses effects, including deletions, from the suspended tree', () => {

784 + const {useState} = React;

785 +

786 + let setTab;

787 + function App() {

788 + const [tab, _setTab] = useState(0);

789 + setTab = _setTab;

790 +

791 + return (

792 + <Suspense fallback={<Text text="Loading..." />}>

793 + <AsyncText key={tab} text={'Tab: ' + tab} ms={1000} />

794 + <Text key={tab + 'sibling'} text=" + sibling" />

795 + </Suspense>

796 + );

797 + }

798 +

799 + const root = ReactTestRenderer.create(<App />);

800 + expect(ReactTestRenderer).toHaveYielded([

801 + 'Suspend! [Tab: 0]',

802 + ' + sibling',

803 + 'Loading...',

804 + ]);

805 + expect(root).toMatchRenderedOutput('Loading...');

806 + jest.advanceTimersByTime(1000);

807 + expect(ReactTestRenderer).toHaveYielded([

808 + 'Promise resolved [Tab: 0]',

809 + 'Tab: 0',

810 + ]);

811 + expect(root).toMatchRenderedOutput('Tab: 0 + sibling');

812 +

813 + setTab(1);

814 + expect(ReactTestRenderer).toHaveYielded([

815 + 'Suspend! [Tab: 1]',

816 + ' + sibling',

817 + 'Loading...',

818 + ]);

819 + expect(root).toMatchRenderedOutput('Loading...');

820 + jest.advanceTimersByTime(1000);

821 + expect(ReactTestRenderer).toHaveYielded([

822 + 'Promise resolved [Tab: 1]',

823 + 'Tab: 1',

824 + ]);

825 + expect(root).toMatchRenderedOutput('Tab: 1 + sibling');

826 +

827 + setTab(2);

828 + expect(ReactTestRenderer).toHaveYielded([

829 + 'Suspend! [Tab: 2]',

830 + ' + sibling',

831 + 'Loading...',

832 + ]);

833 + expect(root).toMatchRenderedOutput('Loading...');

834 + jest.advanceTimersByTime(1000);

835 + expect(ReactTestRenderer).toHaveYielded([

836 + 'Promise resolved [Tab: 2]',

837 + 'Tab: 2',

838 + ]);

839 + expect(root).toMatchRenderedOutput('Tab: 2 + sibling');

840 + });

841 });

842 });