Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Synchronous Suspense] Reuse deletions from primary tree #14133

Merged
merged 1 commit into from Nov 7, 2018
Merged

[Synchronous Suspense] Reuse deletions from primary tree #14133

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
76 changes: 10 additions & 66 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
View file
Open in desktop
Expand Up @@ -1127,11 +1127,7 @@ function updateSuspenseComponent(
progressedState !== null
? (workInProgress.child: any).child
: (workInProgress.child: any);
reuseProgressedPrimaryChild(
workInProgress,
primaryChildFragment,
progressedPrimaryChild,
);
primaryChildFragment.child = progressedPrimaryChild;
}

const fallbackChildFragment = createFiberFromFragment(
Expand Down Expand Up @@ -1186,11 +1182,7 @@ function updateSuspenseComponent(
? (workInProgress.child: any).child
: (workInProgress.child: any);
if (progressedPrimaryChild !== currentPrimaryChildFragment.child) {
reuseProgressedPrimaryChild(
workInProgress,
primaryChildFragment,
progressedPrimaryChild,
);
primaryChildFragment.child = progressedPrimaryChild;
}
}

Expand All @@ -1213,20 +1205,19 @@ function updateSuspenseComponent(
// and remove the intermediate fragment fiber.
const nextPrimaryChildren = nextProps.children;
const currentPrimaryChild = currentPrimaryChildFragment.child;
const currentFallbackChild = currentFallbackChildFragment.child;
const primaryChild = reconcileChildFibers(
workInProgress,
currentPrimaryChild,
nextPrimaryChildren,
renderExpirationTime,
);
// Delete the fallback children.
reconcileChildFibers(
workInProgress,
currentFallbackChild,
null,
renderExpirationTime,
);

// If this render doesn't suspend, we need to delete the fallback
// children. Wait until the complete phase, after we've confirmed the
// fallback is no longer needed.
// TODO: Would it be better to store the fallback fragment on
// the stateNode?

// Continue rendering the children, like we normally do.
child = next = primaryChild;
}
Expand Down Expand Up @@ -1258,11 +1249,7 @@ function updateSuspenseComponent(
progressedState !== null
? (workInProgress.child: any).child
: (workInProgress.child: any);
reuseProgressedPrimaryChild(
workInProgress,
primaryChildFragment,
progressedPrimaryChild,
);
primaryChildFragment.child = progressedPrimaryChild;
}

// Create a fragment from the fallback children, too.
Expand Down Expand Up @@ -1298,49 +1285,6 @@ function updateSuspenseComponent(
return next;
}

function reuseProgressedPrimaryChild(
workInProgress: Fiber,
primaryChildFragment: Fiber,
progressedChild: Fiber | null,
) {
// This function is only called outside concurrent mode. Usually, if a work-
// in-progress primary tree suspends, we throw it out and revert back to
// current. Outside concurrent mode, though, we commit the suspended work-in-
// progress, even though it didn't complete. This function reuses the children
// and transfers the effects.
let child = (primaryChildFragment.child = progressedChild);
while (child !== null) {
// Ensure that the first and last effect of the parent corresponds
// to the children's first and last effect.
if (primaryChildFragment.firstEffect === null) {
primaryChildFragment.firstEffect = child.firstEffect;
}
if (child.lastEffect !== null) {
if (primaryChildFragment.lastEffect !== null) {
primaryChildFragment.lastEffect.nextEffect = child.firstEffect;
}
primaryChildFragment.lastEffect = child.lastEffect;
}

// Append all the effects of the subtree and this fiber onto the effect
// list of the parent. The completion order of the children affects the
// side-effect order.
if (child.effectTag > PerformedWork) {
if (primaryChildFragment.lastEffect !== null) {
primaryChildFragment.lastEffect.nextEffect = child;
} else {
primaryChildFragment.firstEffect = child;
}
primaryChildFragment.lastEffect = child;
}
child.return = primaryChildFragment;
child = child.sibling;
}

workInProgress.firstEffect = primaryChildFragment.firstEffect;
workInProgress.lastEffect = primaryChildFragment.lastEffect;
}

function updatePortalComponent(
current: Fiber | null,
workInProgress: Fiber,
Expand Down
20 changes: 19 additions & 1 deletion packages/react-reconciler/src/ReactFiberCompleteWork.js
View file
Open in desktop
Expand Up @@ -82,6 +82,7 @@ import {
popHydrationState,
} from './ReactFiberHydrationContext';
import {ConcurrentMode, NoContext} from './ReactTypeOfMode';
import {reconcileChildFibers} from './ReactChildFiber';

function markUpdate(workInProgress: Fiber) {
// Tag the fiber with an update effect. This turns a Placement into
Expand Down Expand Up @@ -700,12 +701,29 @@ function completeWork(
if ((workInProgress.effectTag & DidCapture) !== NoEffect) {
// Something suspended. Re-render with the fallback children.
workInProgress.expirationTime = renderExpirationTime;
workInProgress.firstEffect = workInProgress.lastEffect = null;
// Do not reset the effect list.
return workInProgress;
}

const nextDidTimeout = nextState !== null;
const prevDidTimeout = current !== null && current.memoizedState !== null;

if (current !== null && !nextDidTimeout && prevDidTimeout) {
// We just switched from the fallback to the normal children. Delete
// the fallback.
// TODO: Would it be better to store the fallback fragment on
// the stateNode during the begin phase?
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would it be better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid the confusing series of checks this comment is wrapped in. Could be replaced by checking if the stateNode is null.

I'm not sure that's actually better though. Kinda gross either way.

const currentFallbackChild: Fiber | null = (current.child: any).sibling;
if (currentFallbackChild !== null) {
reconcileChildFibers(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be simpler if instead of deleting the fallback we kept it hidden? Although that seems bad from memory perspective.

Copy link
Collaborator Author

@acdlite acdlite Nov 7, 2018 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be simpler because we would always render two fragments, instead of switching between fragments and no fragments like we do now. That's why updateSuspenseComponent has so much code in it; we don't do this type of custom reconciliation anywhere else, yet. Combined with the other weird things we do with Suspense (committing incomplete trees in sync mode, hiding instead of deleting after a timeout) it's gotten pretty complex. Trade off still seems worth it, but unfortunately that means it might take longer to flush out all these edge cases.

(I keep trying to think of a good fuzz test, but I can't. Haven't given up though.)

workInProgress,
currentFallbackChild,
null,
renderExpirationTime,
);
}
}

// The children either timed out after previously being visible, or
// were restored after previously being hidden. Schedule an effect
// to update their visiblity.
Expand Down
1 change: 0 additions & 1 deletion packages/react-reconciler/src/ReactFiberScheduler.js
View file
Open in desktop
Expand Up @@ -993,7 +993,6 @@ function completeUnitOfWork(workInProgress: Fiber): Fiber | null {

if (nextUnitOfWork !== null) {
// Completing this fiber spawned new work. Work on that next.
nextUnitOfWork.firstEffect = nextUnitOfWork.lastEffect = null;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this was the thing that caused us to "forget" about deletions?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

return nextUnitOfWork;
}

Expand Down
59 changes: 59 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js
View file
Open in desktop
Expand Up @@ -779,5 +779,64 @@ describe('ReactSuspense', () => {
expect(root).toFlushAndYield(['Child 1', 'Child 2']);
expect(root).toMatchRenderedOutput(['Child 1', 'Child 2'].join(''));
});

it('reuses effects, including deletions, from the suspended tree', () => {
const {useState} = React;

let setTab;
function App() {
const [tab, _setTab] = useState(0);
setTab = _setTab;

return (
<Suspense fallback={<Text text="Loading..." />}>
<AsyncText key={tab} text={'Tab: ' + tab} ms={1000} />
<Text key={tab + 'sibling'} text=" + sibling" />
</Suspense>
);
}

const root = ReactTestRenderer.create(<App />);
expect(ReactTestRenderer).toHaveYielded([
'Suspend! [Tab: 0]',
' + sibling',
'Loading...',
]);
expect(root).toMatchRenderedOutput('Loading...');
jest.advanceTimersByTime(1000);
expect(ReactTestRenderer).toHaveYielded([
'Promise resolved [Tab: 0]',
'Tab: 0',
]);
expect(root).toMatchRenderedOutput('Tab: 0 + sibling');

setTab(1);
expect(ReactTestRenderer).toHaveYielded([
'Suspend! [Tab: 1]',
' + sibling',
'Loading...',
]);
expect(root).toMatchRenderedOutput('Loading...');
jest.advanceTimersByTime(1000);
expect(ReactTestRenderer).toHaveYielded([
'Promise resolved [Tab: 1]',
'Tab: 1',
]);
expect(root).toMatchRenderedOutput('Tab: 1 + sibling');

setTab(2);
expect(ReactTestRenderer).toHaveYielded([
'Suspend! [Tab: 2]',
' + sibling',
'Loading...',
]);
expect(root).toMatchRenderedOutput('Loading...');
jest.advanceTimersByTime(1000);
expect(ReactTestRenderer).toHaveYielded([
'Promise resolved [Tab: 2]',
'Tab: 2',
]);
expect(root).toMatchRenderedOutput('Tab: 2 + sibling');
});
});
});