Skip to content

Commit

Permalink
Prerendering should not cancel a pending commit
Browse files Browse the repository at this point in the history
If there's a pending commit that's expected to run within an short
amount of time, we should not cancel it in favor of prerendering. We
should wait for the commit to finish before prerendering.

This does not apply to commits that are suspended indefinitely, like
when you suspend outside of a Suspense boundary, or in the shell during
a transition. Because those cases do not represent a complete tree.

There's one special case that we intentionally (for now) don't handle,
which is Suspensey CSS. These are also expected to resolve quickly,
because of preloading, but theoretically they could block forever like
in a normal "suspend indefinitely" scenario. In the future, we should
consider only blocking for up to some time limit before discarding the
commit in favor of prerendering.
  • Loading branch information
acdlite committed Sep 10, 2024
1 parent c4cc54a commit 839d2d8
Show file tree
Hide file tree
Showing 13 changed files with 362 additions and 254 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -226,10 +226,14 @@ describe('ReactCache', () => {
await waitForPaint(['Suspend! [1]', 'Loading...']);
jest.advanceTimersByTime(100);
assertLog(['Promise resolved [1]']);
await waitForAll([1, 'Suspend! [2]', 1, 'Suspend! [2]', 'Suspend! [3]']);
await waitForAll([1, 'Suspend! [2]']);

jest.advanceTimersByTime(100);
assertLog(['Promise resolved [2]', 'Promise resolved [3]']);
assertLog(['Promise resolved [2]']);
await waitForAll([1, 2, 'Suspend! [3]']);

jest.advanceTimersByTime(100);
assertLog(['Promise resolved [3]']);
await waitForAll([1, 2, 3]);

await act(() => jest.advanceTimersByTime(100));
Expand Down
38 changes: 32 additions & 6 deletions packages/react-reconciler/src/ReactFiberLane.js
Original file line number Diff line number Diff line change
Expand Up @@ -232,6 +232,28 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
const pingedLanes = root.pingedLanes;
const warmLanes = root.warmLanes;

// finishedLanes represents a completed tree that is ready to commit.
//
// It's not worth doing discarding the completed tree in favor of performing
// speculative work. So always check this before deciding to warm up
// the siblings.
//
// Note that this is not set in a "suspend indefinitely" scenario, like when
// suspending outside of a Suspense boundary, or in the shell during a
// transition — only in cases where we are very likely to commit the tree in
// a brief amount of time (i.e. below the "Just Noticeable Difference"
// threshold).
//
// TODO: finishedLanes is also set when a Suspensey resource, like CSS or
// images, suspends during the commit phase. (We could detect that here by
// checking for root.cancelPendingCommit.) These are also expected to resolve
// quickly, because of preloading, but theoretically they could block forever
// like in a normal "suspend indefinitely" scenario. In the future, we should
// consider only blocking for up to some time limit before switching to
// prerendering. If we do discard a pending commit, then the commit phase
// callback should act as a ping to try the original render again.
const rootHasPendingCommit = root.finishedLanes !== NoLanes;

// Do not work on any idle work until all the non-idle work has finished,
// even if the work is suspended.
const nonIdlePendingLanes = pendingLanes & NonIdleLanes;
Expand All @@ -247,9 +269,11 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
nextLanes = getHighestPriorityLanes(nonIdlePingedLanes);
} else {
// Nothing has been pinged. Check for lanes that need to be prewarmed.
const lanesToPrewarm = nonIdlePendingLanes & ~warmLanes;
if (lanesToPrewarm !== NoLanes) {
nextLanes = getHighestPriorityLanes(lanesToPrewarm);
if (!rootHasPendingCommit) {
const lanesToPrewarm = nonIdlePendingLanes & ~warmLanes;
if (lanesToPrewarm !== NoLanes) {
nextLanes = getHighestPriorityLanes(lanesToPrewarm);
}
}
}
}
Expand All @@ -269,9 +293,11 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
nextLanes = getHighestPriorityLanes(pingedLanes);
} else {
// Nothing has been pinged. Check for lanes that need to be prewarmed.
const lanesToPrewarm = pendingLanes & ~warmLanes;
if (lanesToPrewarm !== NoLanes) {
nextLanes = getHighestPriorityLanes(lanesToPrewarm);
if (!rootHasPendingCommit) {
const lanesToPrewarm = pendingLanes & ~warmLanes;
if (lanesToPrewarm !== NoLanes) {
nextLanes = getHighestPriorityLanes(lanesToPrewarm);
}
}
}
}
Expand Down
23 changes: 19 additions & 4 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -997,8 +997,6 @@ export function performConcurrentWorkOnRoot(

// We now have a consistent tree. The next step is either to commit it,
// or, if something suspended, wait to commit it after a timeout.
root.finishedWork = finishedWork;
root.finishedLanes = lanes;
finishConcurrentRender(root, exitStatus, finishedWork, lanes);
}
break;
Expand Down Expand Up @@ -1142,6 +1140,12 @@ function finishConcurrentRender(
}
}

// Only set these if we have a complete tree that is ready to be committed.
// We use these fields to determine later whether or not the work should be
// discarded for a fresh render attempt.
root.finishedWork = finishedWork;
root.finishedLanes = lanes;

if (shouldForceFlushFallbacksInDEV()) {
// We're inside an `act` scope. Commit immediately.
commitRoot(
Expand Down Expand Up @@ -1174,8 +1178,11 @@ function finishConcurrentRender(

const nextLanes = getNextLanes(root, NoLanes);
if (nextLanes !== NoLanes) {
// There's additional work we can do on this root. We might as well
// attempt to work on that while we're suspended.
// There are additional updates we can do on this root. We might as
// well attempt to work on that while we're suspended.
//
// Notably, this bailout does not occur if the only thing remaining is
// Suspense retries.
return;
}

Expand Down Expand Up @@ -2226,6 +2233,14 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) {
workInProgressTransitions = getTransitionsForLanes(root, lanes);
resetRenderTimer();
prepareFreshStack(root, lanes);
} else {
// This is a continuation of an existing work-in-progress.
//
// If we were previously in prerendering mode, check if we received any new
// data during an interleaved event.
if (workInProgressRootIsPrerendering) {
workInProgressRootIsPrerendering = checkIfRootIsPrerendering(root, lanes);
}
}

if (__DEV__) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4207,13 +4207,7 @@ describe('ReactHooksWithNoopRenderer', () => {
await act(async () => {
await resolveText('A');
});
assertLog([
'Promise resolved [A]',
'A',
'Suspend! [B]',

...(gate('enableSiblingPrerendering') ? ['A', 'Suspend! [B]'] : []),
]);
assertLog(['Promise resolved [A]', 'A', 'Suspend! [B]']);

await act(() => {
root.render(null);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -198,11 +198,7 @@ describe('ReactLazy', () => {

await resolveFakeImport(Foo);

await waitForAll([
'Foo',

...(gate('enableSiblingPrerendering') ? ['Foo'] : []),
]);
await waitForAll(['Foo']);
expect(root).not.toMatchRenderedOutput('FooBar');

await act(() => resolveFakeImport(Bar));
Expand Down
Loading

0 comments on commit 839d2d8

Please sign in to comment.