Skip to content

Commit

Permalink
[SuspenseList] Fix bugs with dropped Promises (#17082)
Browse files Browse the repository at this point in the history
* Transfer any pending promises from inner boundary to list

For non-hidden modes, this boundary should commit so this shouldn't be
needed but the nested boundary can make a second pass which forces these
to be recreated without resuspending. In this case, the outer list assumes
that it can collect the inner promises to still rerender if needed.

* Propagate suspense "context" change to nested SuspenseLists

This means that we always rerender any nested SuspenseLists together.

This bug looks similar to the previous one but is not based on the lack of
retry but that the retry only happens on the outer boundary but the inner
doesn't get a retry ping since it didn't know about its own promise after
the second pass.
  • Loading branch information
sebmarkbage committed Oct 14, 2019
1 parent 75955bf commit d5b54d0
Show file tree
Hide file tree
Showing 3 changed files with 206 additions and 17 deletions.
33 changes: 22 additions & 11 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -2121,6 +2121,20 @@ function updateDehydratedSuspenseComponent(
}
}

function scheduleWorkOnFiber(
fiber: Fiber,
renderExpirationTime: ExpirationTime,
) {
if (fiber.expirationTime < renderExpirationTime) {
fiber.expirationTime = renderExpirationTime;
}
let alternate = fiber.alternate;
if (alternate !== null && alternate.expirationTime < renderExpirationTime) {
alternate.expirationTime = renderExpirationTime;
}
scheduleWorkOnParentPath(fiber.return, renderExpirationTime);
}

function propagateSuspenseContextChange(
workInProgress: Fiber,
firstChild: null | Fiber,
Expand All @@ -2134,18 +2148,15 @@ function propagateSuspenseContextChange(
if (node.tag === SuspenseComponent) {
const state: SuspenseState | null = node.memoizedState;
if (state !== null) {
if (node.expirationTime < renderExpirationTime) {
node.expirationTime = renderExpirationTime;
}
let alternate = node.alternate;
if (
alternate !== null &&
alternate.expirationTime < renderExpirationTime
) {
alternate.expirationTime = renderExpirationTime;
}
scheduleWorkOnParentPath(node.return, renderExpirationTime);
scheduleWorkOnFiber(node, renderExpirationTime);
}
} else if (node.tag === SuspenseListComponent) {
// If the tail is hidden there might not be an Suspense boundaries
// to schedule work on. In this case we have to schedule it on the
// list itself.
// We don't have to traverse to the children of the list since
// the list will propagate the change when it rerenders.
scheduleWorkOnFiber(node, renderExpirationTime);
} else if (node.child !== null) {
node.child.return = node;
node = node.child;
Expand Down
15 changes: 9 additions & 6 deletions packages/react-reconciler/src/ReactFiberCompleteWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -1083,19 +1083,22 @@ function completeWork(
if (suspended !== null) {
workInProgress.effectTag |= DidCapture;
didSuspendAlready = true;

// Ensure we transfer the update queue to the parent so that it doesn't
// get lost if this row ends up dropped during a second pass.
let newThennables = suspended.updateQueue;
if (newThennables !== null) {
workInProgress.updateQueue = newThennables;
workInProgress.effectTag |= Update;
}

cutOffTailIfNeeded(renderState, true);
// This might have been modified.
if (
renderState.tail === null &&
renderState.tailMode === 'hidden'
) {
// We need to delete the row we just rendered.
// Ensure we transfer the update queue to the parent.
let newThennables = suspended.updateQueue;
if (newThennables !== null) {
workInProgress.updateQueue = newThennables;
workInProgress.effectTag |= Update;
}
// Reset the effect list to what it w as before we rendered this
// child. The nested children have already appended themselves.
let lastEffect = (workInProgress.lastEffect =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1826,6 +1826,181 @@ describe('ReactSuspenseList', () => {
);
});

it('eventually resolves a nested forwards suspense list', async () => {
let B = createAsyncText('B');

function Foo() {
return (
<SuspenseList revealOrder="together">
<SuspenseList revealOrder="forwards">
<Suspense fallback={<Text text="Loading A" />}>
<Text text="A" />
</Suspense>
<Suspense fallback={<Text text="Loading B" />}>
<B />
</Suspense>
<Suspense fallback={<Text text="Loading C" />}>
<Text text="C" />
</Suspense>
</SuspenseList>
<Suspense fallback={<Text text="Loading D" />}>
<Text text="D" />
</Suspense>
</SuspenseList>
);
}

ReactNoop.render(<Foo />);

expect(Scheduler).toFlushAndYield([
'A',
'Suspend! [B]',
'Loading B',
'Loading C',
'D',
// The second pass forces the fallbacks
'Loading A',
'Loading B',
'Loading C',
'Loading D',
]);

expect(ReactNoop).toMatchRenderedOutput(
<>
<span>Loading A</span>
<span>Loading B</span>
<span>Loading C</span>
<span>Loading D</span>
</>,
);

await B.resolve();

expect(Scheduler).toFlushAndYield(['A', 'B', 'C', 'D']);

expect(ReactNoop).toMatchRenderedOutput(
<>
<span>A</span>
<span>B</span>
<span>C</span>
<span>D</span>
</>,
);
});

it('eventually resolves a nested forwards suspense list with a hidden tail', async () => {
let B = createAsyncText('B');

function Foo() {
return (
<SuspenseList revealOrder="together">
<SuspenseList revealOrder="forwards" tail="hidden">
<Suspense fallback={<Text text="Loading A" />}>
<Text text="A" />
</Suspense>
<Suspense fallback={<Text text="Loading B" />}>
<B />
</Suspense>
</SuspenseList>
<Suspense fallback={<Text text="Loading C" />}>
<Text text="C" />
</Suspense>
</SuspenseList>
);
}

ReactNoop.render(<Foo />);

expect(Scheduler).toFlushAndYield([
'A',
'Suspend! [B]',
'Loading B',
'C',
'Loading C',
]);

expect(ReactNoop).toMatchRenderedOutput(<span>Loading C</span>);

await B.resolve();

expect(Scheduler).toFlushAndYield(['A', 'B', 'C']);

expect(ReactNoop).toMatchRenderedOutput(
<>
<span>A</span>
<span>B</span>
<span>C</span>
</>,
);
});

it('eventually resolves two nested forwards suspense list with a hidden tail', async () => {
let B = createAsyncText('B');

function Foo({showB}) {
return (
<SuspenseList revealOrder="forwards">
<SuspenseList revealOrder="forwards" tail="hidden">
<Suspense fallback={<Text text="Loading A" />}>
<Text text="A" />
</Suspense>
{showB ? (
<Suspense fallback={<Text text="Loading B" />}>
<B />
</Suspense>
) : null}
</SuspenseList>
<Suspense fallback={<Text text="Loading C" />}>
<Text text="C" />
</Suspense>
</SuspenseList>
);
}

ReactNoop.render(<Foo showB={false} />);

expect(Scheduler).toFlushAndYield(['A', 'C']);

expect(ReactNoop).toMatchRenderedOutput(
<>
<span>A</span>
<span>C</span>
</>,
);

// Showing the B later means that C has already committed
// so we're now effectively in "together" mode for the head.
ReactNoop.render(<Foo showB={true} />);

expect(Scheduler).toFlushAndYield([
'A',
'Suspend! [B]',
'Loading B',
'C',
'A',
'C',
]);

expect(ReactNoop).toMatchRenderedOutput(
<>
<span>A</span>
<span>C</span>
</>,
);

await B.resolve();

expect(Scheduler).toFlushAndYield(['B']);

expect(ReactNoop).toMatchRenderedOutput(
<>
<span>A</span>
<span>B</span>
<span>C</span>
</>,
);
});

it('can do unrelated adjacent updates', async () => {
let updateAdjacent;
function Adjacent() {
Expand Down

0 comments on commit d5b54d0

Please sign in to comment.