Skip to content

Commit

Permalink
Fix issue with tail exceeding the CPU time limit
Browse files Browse the repository at this point in the history
We used to assume that this didn't suspend but this branch happens in
both cases. This fixes it so that we first check if we suspended.

Now we can fix the tail so that it always render an additional fallback
in this scenario.
  • Loading branch information
sebmarkbage committed Jul 2, 2019
1 parent 6cca5ff commit 52a6de9
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 5 deletions.
11 changes: 6 additions & 5 deletions packages/react-reconciler/src/ReactFiberCompleteWork.js
Original file line number Diff line number Diff line change
Expand Up @@ -1035,7 +1035,11 @@ function completeWork(
} else {
// Append the rendered row to the child list.
if (!didSuspendAlready) {
if (
if (isShowingAnyFallbacks(renderedTail)) {
workInProgress.effectTag |= DidCapture;
didSuspendAlready = true;
cutOffTailIfNeeded(renderState);
} else if (
now() > renderState.tailExpiration &&
renderExpirationTime > Never
) {
Expand All @@ -1045,6 +1049,7 @@ function completeWork(
workInProgress.effectTag |= DidCapture;
didSuspendAlready = true;

renderState.rendering = null;
cutOffTailIfNeeded(renderState);

// Since nothing actually suspended, there will nothing to ping this
Expand All @@ -1058,10 +1063,6 @@ function completeWork(
if (enableSchedulerTracing) {
markSpawnedWork(nextPriority);
}
} else if (isShowingAnyFallbacks(renderedTail)) {
workInProgress.effectTag |= DidCapture;
didSuspendAlready = true;
cutOffTailIfNeeded(renderState);
}
}
if (renderState.isBackwards) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1197,6 +1197,72 @@ describe('ReactSuspenseList', () => {
]);
});

it('renders one "collapsed" fallback even if CPU time elapsed', async () => {
function Foo() {
return (
<SuspenseList revealOrder="forwards" tail="collapsed">
<Suspense fallback={<Text text="Loading A" />}>
<Text text="A" />
</Suspense>
<Suspense fallback={<Text text="Loading B" />}>
<Text text="B" />
</Suspense>
<Suspense fallback={<Text text="Loading C" />}>
<Text text="C" />
</Suspense>
<Suspense fallback={<Text text="Loading D" />}>
<Text text="D" />
</Suspense>
</SuspenseList>
);
}

// This render is only CPU bound. Nothing suspends.
ReactNoop.render(<Foo />);

expect(Scheduler).toFlushAndYieldThrough(['A']);

Scheduler.unstable_advanceTime(300);
jest.advanceTimersByTime(300);

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

Scheduler.unstable_advanceTime(300);
jest.advanceTimersByTime(300);

// We've still not been able to show anything on the screen even though
// we have two items ready.
expect(ReactNoop).toMatchRenderedOutput(null);

// Time has now elapsed for so long that we're just going to give up
// rendering the rest of the content. So that we can at least show
// something.
expect(Scheduler).toFlushAndYieldThrough([
'Loading C',
'C', // I'll flush through into the next render so that the first commits.
]);

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

// Then we do a second pass to commit the last two items.
expect(Scheduler).toFlushAndYield(['D']);

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

it('adding to the middle does not collapse insertions (forwards)', async () => {
let A = createAsyncText('A');
let B = createAsyncText('B');
Expand Down

0 comments on commit 52a6de9

Please sign in to comment.