Skip to content

Commit

Permalink
Allow transitions to interrupt Suspensey commits (#26531)
Browse files Browse the repository at this point in the history 
I originally made it so that a Suspensey commit — i.e. a commit that's
waiting for a stylesheet, image, or font to load before proceeding —
could not be interrupted by transitions. My reasoning was that Suspensey
commits always time out after a short interval, anyway, so if the
incoming update isn't urgent, it's better to wait to commit the current
frame instead of throwing it away.

I don't think this rationale was correct, for a few reasons. There are
some cases where we'll suspend for a longer duration, like stylesheets —
it's nearly always a bad idea to show content before its styles have
loaded, so we're going to be extend this timeout to be really long.

But even in the case where the timeout is shorter, like fonts, if you
get a new update, it's possible (even likely) that update will allow us
to avoid showing a fallback, like by navigating to a different page. So
we might as well try.

The behavior now matches our behavior for interrupting a suspended
render phase (i.e. `use`), which makes sense because they're not that
conceptually different.
  • Loading branch information
@acdlite
acdlite committed Mar 31, 2023
1 parent 09c8d25 commit 8888746
Show file tree
Hide file tree
Showing 4 changed files with 40 additions and 65 deletions.
67 changes: 26 additions & 41 deletions packages/react-dom/src/__tests__/ReactDOMFloat-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3216,7 +3216,7 @@ body {
);
});

it('can start a new suspended commit after a previous one finishes', async () => {
it('can interrupt a suspended commit with a new transition', async () => {
function App({children}) {
return (
<html>
Expand All @@ -3225,81 +3225,66 @@ body {
);
}
const root = ReactDOMClient.createRoot(document);
root.render(<App />);
root.render(<App>(empty)</App>);

// Start a transition to "A"
React.startTransition(() => {
root.render(
<App>
hello
<link rel="stylesheet" href="foo" precedence="default" />
A
<link rel="stylesheet" href="A" precedence="default" />
</App>,
);
});
await waitForAll([]);

// "A" hasn't loaded yet, so we remain on the initial UI. Its preload
// has been inserted into the head, though.
expect(getMeaningfulChildren(document)).toEqual(
<html>
<head>
<link rel="preload" href="foo" as="style" />
<link rel="preload" href="A" as="style" />
</head>
<body />
<body>(empty)</body>
</html>,
);

// Interrupt the "A" transition with a new one, "B"
React.startTransition(() => {
root.render(
<App>
hello2
{null}
<link rel="stylesheet" href="bar" precedence="default" />
B
<link rel="stylesheet" href="B" precedence="default" />
</App>,
);
});
await waitForAll([]);
expect(getMeaningfulChildren(document)).toEqual(
<html>
<head>
<link rel="preload" href="foo" as="style" />
</head>
<body />
</html>,
);

loadPreloads();
loadStylesheets();
assertLog(['load preload: foo', 'load stylesheet: foo']);
// Still on the initial UI because "B" hasn't loaded, but its preload
// is now in the head, too.
expect(getMeaningfulChildren(document)).toEqual(
<html>
<head>
<link rel="stylesheet" href="foo" data-precedence="default" />
<link rel="preload" href="foo" as="style" />
<link rel="preload" href="A" as="style" />
<link rel="preload" href="B" as="style" />
</head>
<body>hello</body>
<body>(empty)</body>
</html>,
);

// The second update should process now
await waitForAll([]);
expect(getMeaningfulChildren(document)).toEqual(
<html>
<head>
<link rel="stylesheet" href="foo" data-precedence="default" />
<link rel="preload" href="foo" as="style" />
<link rel="preload" href="bar" as="style" />
</head>
<body>hello</body>
</html>,
);
// Finish loading
loadPreloads();
loadStylesheets();
assertLog(['load preload: bar', 'load stylesheet: bar']);
assertLog(['load preload: A', 'load preload: B', 'load stylesheet: B']);
// The "B" transition has finished.
expect(getMeaningfulChildren(document)).toEqual(
<html>
<head>
<link rel="stylesheet" href="foo" data-precedence="default" />
<link rel="stylesheet" href="bar" data-precedence="default" />
<link rel="preload" href="foo" as="style" />
<link rel="preload" href="bar" as="style" />
<link rel="stylesheet" href="B" data-precedence="default" />
<link rel="preload" href="A" as="style" />
<link rel="preload" href="B" as="style" />
</head>
<body>hello2</body>
<body>B</body>
</html>,
);
});
Expand Down
11 changes: 6 additions & 5 deletions packages/react-reconciler/src/ReactFiberRootScheduler.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import {
SyncLane,
getHighestPriorityLane,
getNextLanes,
includesOnlyNonUrgentLanes,
includesSyncLane,
markStarvedLanesAsExpired,
} from './ReactFiberLane';
Expand Down Expand Up @@ -292,14 +291,16 @@ function scheduleTaskForRootDuringMicrotask(

const existingCallbackNode = root.callbackNode;
if (
// Check if there's nothing to work on
nextLanes === NoLanes ||
// If this root is currently suspended and waiting for data to resolve, don't
// schedule a task to render it. We'll either wait for a ping, or wait to
// receive an update.
(isWorkLoopSuspendedOnData() && root === workInProgressRoot) ||
// We should only interrupt a pending commit if the new update
// is urgent.
(root.cancelPendingCommit !== null && includesOnlyNonUrgentLanes(nextLanes))
//
// Suspended render phase
(root === workInProgressRoot && isWorkLoopSuspendedOnData()) ||
// Suspended commit phase
root.cancelPendingCommit !== null
) {
// Fast path: There's nothing to work on.
if (existingCallbackNode !== null) {
Expand Down
7 changes: 5 additions & 2 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -725,8 +725,11 @@ export function scheduleUpdateOnFiber(
// Check if the work loop is currently suspended and waiting for data to
// finish loading.
if (
workInProgressSuspendedReason === SuspendedOnData &&
root === workInProgressRoot
// Suspended render phase
(root === workInProgressRoot &&
workInProgressSuspendedReason === SuspendedOnData) ||
// Suspended commit phase
root.cancelPendingCommit !== null
) {
// The incoming update might unblock the current render. Interrupt the
// current attempt and restart from the top.
Expand Down
20 changes: 3 additions & 17 deletions packages/react-reconciler/src/__tests__/ReactSuspenseyCommitPhase-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -137,15 +137,15 @@ describe('ReactSuspenseyCommitPhase', () => {
// Nothing showing yet.
expect(root).toMatchRenderedOutput(null);

// If there's an urgent update, it should interrupt the suspended commit.
// If there's an update, it should interrupt the suspended commit.
await act(() => {
root.render(<Text text="Something else" />);
});
assertLog(['Something else']);
expect(root).toMatchRenderedOutput('Something else');
});

test('a non-urgent update does not interrupt a suspended commit', async () => {
test('a transition update interrupts a suspended commit', async () => {
const root = ReactNoop.createRoot();

// Mount an image. This transition will suspend because it's not inside a
Expand All @@ -159,26 +159,12 @@ describe('ReactSuspenseyCommitPhase', () => {
// Nothing showing yet.
expect(root).toMatchRenderedOutput(null);

// If there's another transition update, it should not interrupt the
// suspended commit.
// If there's an update, it should interrupt the suspended commit.
await act(() => {
startTransition(() => {
root.render(<Text text="Something else" />);
});
});
// Still suspended.
expect(root).toMatchRenderedOutput(null);

await act(() => {
// Resolving the image should result in an immediate, synchronous commit.
resolveSuspenseyThing('A');
expect(root).toMatchRenderedOutput(<suspensey-thing src="A" />);
});
// Then the second transition is unblocked.
// TODO: Right now the only way to unsuspend a commit early is to proceed
// with the commit even if everything isn't ready. Maybe there should also
// be a way to abort a commit so that it can be interrupted by
// another transition.
assertLog(['Something else']);
expect(root).toMatchRenderedOutput('Something else');
});
Expand Down

0 comments on commit 8888746

Please sign in to comment.