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

Allow transitions to interrupt Suspensey commits #26531

Merged
merged 1 commit into from
Mar 31, 2023
Merged

Allow transitions to interrupt Suspensey commits #26531

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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 26 additions & 41 deletions packages/react-dom/src/__tests__/ReactDOMFloat-test.js
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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