Skip to content

Commit 818fe95

Browse files
acdliteacdlite
committed
Prerendering should not cancel a pending commit
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.
1 parent dc3ccc2 commit 818fe95

15 files changed

+478
-266
lines changed

‎packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,12 @@ describe('ReactCache', () => {
206206
...(gate('enableSiblingPrerendering') ? ['Invalid key type'] : []),
207207
]);
208208
} else {
209-
await waitForAll(['App', 'Loading...']);
209+
await waitForAll([
210+
'App',
211+
'Loading...',
212+
213+
...(gate('enableSiblingPrerendering') ? ['App'] : []),
214+
]);
210215
}
211216
});
212217

@@ -226,10 +231,14 @@ describe('ReactCache', () => {
226231
await waitForPaint(['Suspend! [1]', 'Loading...']);
227232
jest.advanceTimersByTime(100);
228233
assertLog(['Promise resolved [1]']);
229-
await waitForAll([1, 'Suspend! [2]', 1, 'Suspend! [2]', 'Suspend! [3]']);
234+
await waitForAll([1, 'Suspend! [2]']);
235+
236+
jest.advanceTimersByTime(100);
237+
assertLog(['Promise resolved [2]']);
238+
await waitForAll([1, 2, 'Suspend! [3]']);
230239

231240
jest.advanceTimersByTime(100);
232-
assertLog(['Promise resolved [2]', 'Promise resolved [3]']);
241+
assertLog(['Promise resolved [3]']);
233242
await waitForAll([1, 2, 3]);
234243

235244
await act(() => jest.advanceTimersByTime(100));

‎packages/react-devtools-shared/src/__tests__/TimelineProfiler-test.js‎

Lines changed: 94 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,17 @@ import {
1515
normalizeCodeLocInfo,
1616
} from './utils';
1717

18+
import {ReactVersion} from '../../../../ReactVersions';
19+
import semver from 'semver';
20+
21+
// TODO: This is how other DevTools tests access the version but we should find
22+
// a better solution for this
23+
const ReactVersionTestingAgainst = process.env.REACT_VERSION || ReactVersion;
24+
const enableSiblingPrerendering = semver.gte(
25+
ReactVersionTestingAgainst,
26+
'19.0.0',
27+
);
28+
1829
describe('Timeline profiler', () => {
1930
let React;
2031
let Scheduler;
@@ -1651,7 +1662,11 @@ describe('Timeline profiler', () => {
16511662
</React.Suspense>,
16521663
);
16531664

1654-
await waitForAll(['suspended']);
1665+
await waitForAll([
1666+
'suspended',
1667+
1668+
...(enableSiblingPrerendering ? ['suspended'] : []),
1669+
]);
16551670

16561671
Scheduler.unstable_advanceTime(10);
16571672
resolveFn();
@@ -1662,9 +1677,38 @@ describe('Timeline profiler', () => {
16621677
const timelineData = stopProfilingAndGetTimelineData();
16631678

16641679
// Verify the Suspense event and duration was recorded.
1665-
expect(timelineData.suspenseEvents).toHaveLength(1);
1666-
const suspenseEvent = timelineData.suspenseEvents[0];
1667-
expect(suspenseEvent).toMatchInlineSnapshot(`
1680+
if (enableSiblingPrerendering) {
1681+
expect(timelineData.suspenseEvents).toMatchInlineSnapshot(`
1682+
[
1683+
{
1684+
"componentName": "Example",
1685+
"depth": 0,
1686+
"duration": 10,
1687+
"id": "0",
1688+
"phase": "mount",
1689+
"promiseName": "",
1690+
"resolution": "resolved",
1691+
"timestamp": 10,
1692+
"type": "suspense",
1693+
"warning": null,
1694+
},
1695+
{
1696+
"componentName": "Example",
1697+
"depth": 0,
1698+
"duration": 10,
1699+
"id": "0",
1700+
"phase": "mount",
1701+
"promiseName": "",
1702+
"resolution": "resolved",
1703+
"timestamp": 10,
1704+
"type": "suspense",
1705+
"warning": null,
1706+
},
1707+
]
1708+
`);
1709+
} else {
1710+
const suspenseEvent = timelineData.suspenseEvents[0];
1711+
expect(suspenseEvent).toMatchInlineSnapshot(`
16681712
{
16691713
"componentName": "Example",
16701714
"depth": 0,
@@ -1678,10 +1722,13 @@ describe('Timeline profiler', () => {
16781722
"warning": null,
16791723
}
16801724
`);
1725+
}
16811726

16821727
// There should be two batches of renders: Suspeneded and resolved.
16831728
expect(timelineData.batchUIDToMeasuresMap.size).toBe(2);
1684-
expect(timelineData.componentMeasures).toHaveLength(2);
1729+
expect(timelineData.componentMeasures).toHaveLength(
1730+
enableSiblingPrerendering ? 3 : 2,
1731+
);
16851732
});
16861733

16871734
it('should mark concurrent render with suspense that rejects', async () => {
@@ -1708,7 +1755,11 @@ describe('Timeline profiler', () => {
17081755
</React.Suspense>,
17091756
);
17101757

1711-
await waitForAll(['suspended']);
1758+
await waitForAll([
1759+
'suspended',
1760+
1761+
...(enableSiblingPrerendering ? ['suspended'] : []),
1762+
]);
17121763

17131764
Scheduler.unstable_advanceTime(10);
17141765
rejectFn();
@@ -1719,9 +1770,39 @@ describe('Timeline profiler', () => {
17191770
const timelineData = stopProfilingAndGetTimelineData();
17201771

17211772
// Verify the Suspense event and duration was recorded.
1722-
expect(timelineData.suspenseEvents).toHaveLength(1);
1723-
const suspenseEvent = timelineData.suspenseEvents[0];
1724-
expect(suspenseEvent).toMatchInlineSnapshot(`
1773+
if (enableSiblingPrerendering) {
1774+
expect(timelineData.suspenseEvents).toMatchInlineSnapshot(`
1775+
[
1776+
{
1777+
"componentName": "Example",
1778+
"depth": 0,
1779+
"duration": 10,
1780+
"id": "0",
1781+
"phase": "mount",
1782+
"promiseName": "",
1783+
"resolution": "rejected",
1784+
"timestamp": 10,
1785+
"type": "suspense",
1786+
"warning": null,
1787+
},
1788+
{
1789+
"componentName": "Example",
1790+
"depth": 0,
1791+
"duration": 10,
1792+
"id": "0",
1793+
"phase": "mount",
1794+
"promiseName": "",
1795+
"resolution": "rejected",
1796+
"timestamp": 10,
1797+
"type": "suspense",
1798+
"warning": null,
1799+
},
1800+
]
1801+
`);
1802+
} else {
1803+
expect(timelineData.suspenseEvents).toHaveLength(1);
1804+
const suspenseEvent = timelineData.suspenseEvents[0];
1805+
expect(suspenseEvent).toMatchInlineSnapshot(`
17251806
{
17261807
"componentName": "Example",
17271808
"depth": 0,
@@ -1735,10 +1816,13 @@ describe('Timeline profiler', () => {
17351816
"warning": null,
17361817
}
17371818
`);
1819+
}
17381820

17391821
// There should be two batches of renders: Suspeneded and resolved.
17401822
expect(timelineData.batchUIDToMeasuresMap.size).toBe(2);
1741-
expect(timelineData.componentMeasures).toHaveLength(2);
1823+
expect(timelineData.componentMeasures).toHaveLength(
1824+
enableSiblingPrerendering ? 3 : 2,
1825+
);
17421826
});
17431827

17441828
it('should mark cascading class component state updates', async () => {

‎packages/react-reconciler/src/ReactFiberLane.js‎

Lines changed: 33 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -233,6 +233,29 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
233233
const pingedLanes = root.pingedLanes;
234234
const warmLanes = root.warmLanes;
235235

236+
// finishedLanes represents a completed tree that is ready to commit.
237+
//
238+
// It's not worth doing discarding the completed tree in favor of performing
239+
// speculative work. So always check this before deciding to warm up
240+
// the siblings.
241+
//
242+
// Note that this is not set in a "suspend indefinitely" scenario, like when
243+
// suspending outside of a Suspense boundary, or in the shell during a
244+
// transition — only in cases where we are very likely to commit the tree in
245+
// a brief amount of time (i.e. below the "Just Noticeable Difference"
246+
// threshold).
247+
//
248+
// TODO: finishedLanes is also set when a Suspensey resource, like CSS or
249+
// images, suspends during the commit phase. (We could detect that here by
250+
// checking for root.cancelPendingCommit.) These are also expected to resolve
251+
// quickly, because of preloading, but theoretically they could block forever
252+
// like in a normal "suspend indefinitely" scenario. In the future, we should
253+
// consider only blocking for up to some time limit before discarding the
254+
// commit in favor of prerendering. If we do discard a pending commit, then
255+
// the commit phase callback should act as a ping to try the original
256+
// render again.
257+
const rootHasPendingCommit = root.finishedLanes !== NoLanes;
258+
236259
// Do not work on any idle work until all the non-idle work has finished,
237260
// even if the work is suspended.
238261
const nonIdlePendingLanes = pendingLanes & NonIdleLanes;
@@ -248,9 +271,11 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
248271
nextLanes = getHighestPriorityLanes(nonIdlePingedLanes);
249272
} else {
250273
// Nothing has been pinged. Check for lanes that need to be prewarmed.
251-
const lanesToPrewarm = nonIdlePendingLanes & ~warmLanes;
252-
if (lanesToPrewarm !== NoLanes) {
253-
nextLanes = getHighestPriorityLanes(lanesToPrewarm);
274+
if (!rootHasPendingCommit) {
275+
const lanesToPrewarm = nonIdlePendingLanes & ~warmLanes;
276+
if (lanesToPrewarm !== NoLanes) {
277+
nextLanes = getHighestPriorityLanes(lanesToPrewarm);
278+
}
254279
}
255280
}
256281
}
@@ -270,9 +295,11 @@ export function getNextLanes(root: FiberRoot, wipLanes: Lanes): Lanes {
270295
nextLanes = getHighestPriorityLanes(pingedLanes);
271296
} else {
272297
// Nothing has been pinged. Check for lanes that need to be prewarmed.
273-
const lanesToPrewarm = pendingLanes & ~warmLanes;
274-
if (lanesToPrewarm !== NoLanes) {
275-
nextLanes = getHighestPriorityLanes(lanesToPrewarm);
298+
if (!rootHasPendingCommit) {
299+
const lanesToPrewarm = pendingLanes & ~warmLanes;
300+
if (lanesToPrewarm !== NoLanes) {
301+
nextLanes = getHighestPriorityLanes(lanesToPrewarm);
302+
}
276303
}
277304
}
278305
}

‎packages/react-reconciler/src/ReactFiberWorkLoop.js‎

Lines changed: 19 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -997,8 +997,6 @@ export function performConcurrentWorkOnRoot(
997997

998998
// We now have a consistent tree. The next step is either to commit it,
999999
// or, if something suspended, wait to commit it after a timeout.
1000-
root.finishedWork = finishedWork;
1001-
root.finishedLanes = lanes;
10021000
finishConcurrentRender(root, exitStatus, finishedWork, lanes);
10031001
}
10041002
break;
@@ -1142,6 +1140,12 @@ function finishConcurrentRender(
11421140
}
11431141
}
11441142

1143+
// Only set these if we have a complete tree that is ready to be committed.
1144+
// We use these fields to determine later whether or not the work should be
1145+
// discarded for a fresh render attempt.
1146+
root.finishedWork = finishedWork;
1147+
root.finishedLanes = lanes;
1148+
11451149
if (shouldForceFlushFallbacksInDEV()) {
11461150
// We're inside an `act` scope. Commit immediately.
11471151
commitRoot(
@@ -1174,8 +1178,11 @@ function finishConcurrentRender(
11741178

11751179
const nextLanes = getNextLanes(root, NoLanes);
11761180
if (nextLanes !== NoLanes) {
1177-
// There's additional work we can do on this root. We might as well
1178-
// attempt to work on that while we're suspended.
1181+
// There are additional updates we can do on this root. We might as
1182+
// well attempt to work on that while we're suspended.
1183+
//
1184+
// Notably, this bailout does not occur if the only thing remaining is
1185+
// Suspense retries.
11791186
return;
11801187
}
11811188

@@ -2226,6 +2233,14 @@ function renderRootConcurrent(root: FiberRoot, lanes: Lanes) {
22262233
workInProgressTransitions = getTransitionsForLanes(root, lanes);
22272234
resetRenderTimer();
22282235
prepareFreshStack(root, lanes);
2236+
} else {
2237+
// This is a continuation of an existing work-in-progress.
2238+
//
2239+
// If we were previously in prerendering mode, check if we received any new
2240+
// data during an interleaved event.
2241+
if (workInProgressRootIsPrerendering) {
2242+
workInProgressRootIsPrerendering = checkIfRootIsPrerendering(root, lanes);
2243+
}
22292244
}
22302245

22312246
if (__DEV__) {

‎packages/react-reconciler/src/__tests__/DebugTracing-test.internal.js‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -187,12 +187,26 @@ describe('DebugTracing', () => {
187187
`group: ⚛ render (${DEFAULT_LANE_STRING})`,
188188
'log: ⚛ Example suspended',
189189
`groupEnd: ⚛ render (${DEFAULT_LANE_STRING})`,
190+
191+
...(gate('enableSiblingPrerendering')
192+
? [
193+
`group: ⚛ render (${RETRY_LANE_STRING})`,
194+
'log: ⚛ Example suspended',
195+
`groupEnd: ⚛ render (${RETRY_LANE_STRING})`,
196+
]
197+
: []),
190198
]);
191199

192200
logs.splice(0);
193201

194202
await act(async () => await resolveFakeSuspensePromise());
195-
expect(logs).toEqual(['log: ⚛ Example resolved']);
203+
expect(logs).toEqual([
204+
'log: ⚛ Example resolved',
205+
206+
...(gate('enableSiblingPrerendering')
207+
? ['log: ⚛ Example resolved']
208+
: []),
209+
]);
196210
});
197211

198212
// @gate experimental && build === 'development' && enableDebugTracing && enableCPUSuspense

‎packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -4207,13 +4207,7 @@ describe('ReactHooksWithNoopRenderer', () => {
42074207
await act(async () => {
42084208
await resolveText('A');
42094209
});
4210-
assertLog([
4211-
'Promise resolved [A]',
4212-
'A',
4213-
'Suspend! [B]',
4214-
4215-
...(gate('enableSiblingPrerendering') ? ['A', 'Suspend! [B]'] : []),
4216-
]);
4210+
assertLog(['Promise resolved [A]', 'A', 'Suspend! [B]']);
42174211

42184212
await act(() => {
42194213
root.render(null);

‎packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js‎

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -198,11 +198,7 @@ describe('ReactLazy', () => {
198198

199199
await resolveFakeImport(Foo);
200200

201-
await waitForAll([
202-
'Foo',
203-
204-
...(gate('enableSiblingPrerendering') ? ['Foo'] : []),
205-
]);
201+
await waitForAll(['Foo']);
206202
expect(root).not.toMatchRenderedOutput('FooBar');
207203

208204
await act(() => resolveFakeImport(Bar));

0 commit comments

Comments
 (0)