Skip to content

Commit

Permalink
Start prerendering Suspense retries immediately (#30934)
Browse files Browse the repository at this point in the history
When a component suspends and is replaced by a fallback, we should start
prerendering the fallback immediately, even before any new data is
received. During the retry, we can enter prerender mode directly if
we're sure that no new data was received since we last attempted to
render the boundary.

To do this, when completing the fallback, we leave behind a pending
retry lane on the Suspense boundary. Previously we only did this once a
promise resolved, but by assigning a lane during the complete phase, we
will know that there's speculative work to be done.

Then, upon committing the fallback, we mark the retry lane as suspended
— but only if nothing was pinged or updated in the meantime. That allows
us to immediately enter prerender mode (i.e. render without skipping any
siblings) when performing the retry.
  • Loading branch information
acdlite authored Sep 11, 2024
1 parent 1bb0563 commit d6cb4e7
Show file tree
Hide file tree
Showing 29 changed files with 1,906 additions and 462 deletions.
98 changes: 53 additions & 45 deletions packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ let Suspense;
let TextResource;
let textResourceShouldFail;
let waitForAll;
let waitForPaint;
let assertLog;
let waitForThrow;
let act;
Expand All @@ -37,6 +38,7 @@ describe('ReactCache', () => {
waitForAll = InternalTestUtils.waitForAll;
assertLog = InternalTestUtils.assertLog;
waitForThrow = InternalTestUtils.waitForThrow;
waitForPaint = InternalTestUtils.waitForPaint;
act = InternalTestUtils.act;

TextResource = createResource(
Expand Down Expand Up @@ -119,7 +121,12 @@ describe('ReactCache', () => {
const root = ReactNoop.createRoot();
root.render(<App />);

await waitForAll(['Suspend! [Hi]', 'Loading...']);
await waitForAll([
'Suspend! [Hi]',
'Loading...',

...(gate('enableSiblingPrerendering') ? ['Suspend! [Hi]'] : []),
]);

jest.advanceTimersByTime(100);
assertLog(['Promise resolved [Hi]']);
Expand All @@ -138,7 +145,12 @@ describe('ReactCache', () => {
const root = ReactNoop.createRoot();
root.render(<App />);

await waitForAll(['Suspend! [Hi]', 'Loading...']);
await waitForAll([
'Suspend! [Hi]',
'Loading...',

...(gate('enableSiblingPrerendering') ? ['Suspend! [Hi]'] : []),
]);

textResourceShouldFail = true;
let error;
Expand All @@ -148,15 +160,7 @@ describe('ReactCache', () => {
error = e;
}
expect(error.message).toMatch('Failed to load: Hi');
assertLog([
'Promise rejected [Hi]',
'Error! [Hi]',
'Error! [Hi]',

...(gate('enableSiblingPrerendering')
? ['Error! [Hi]', 'Error! [Hi]']
: []),
]);
assertLog(['Promise rejected [Hi]', 'Error! [Hi]', 'Error! [Hi]']);

// Should throw again on a subsequent read
root.render(<App />);
Expand Down Expand Up @@ -187,15 +191,27 @@ describe('ReactCache', () => {

if (__DEV__) {
await expect(async () => {
await waitForAll(['App', 'Loading...']);
await waitForAll([
'App',
'Loading...',

...(gate('enableSiblingPrerendering') ? ['App'] : []),
]);
}).toErrorDev([
'Invalid key type. Expected a string, number, symbol, or ' +
"boolean, but instead received: [ 'Hi', 100 ]\n\n" +
'To use non-primitive values as keys, you must pass a hash ' +
'function as the second argument to createResource().',

...(gate('enableSiblingPrerendering') ? ['Invalid key type'] : []),
]);
} else {
await waitForAll(['App', 'Loading...']);
await waitForAll([
'App',
'Loading...',

...(gate('enableSiblingPrerendering') ? ['App'] : []),
]);
}
});

Expand All @@ -212,13 +228,17 @@ describe('ReactCache', () => {
<AsyncText ms={100} text={3} />
</Suspense>,
);
await waitForAll(['Suspend! [1]', 'Loading...']);
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]']);
await waitForAll([1, 2, 'Suspend! [3]']);

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

await act(() => jest.advanceTimersByTime(100));
Expand All @@ -233,25 +253,18 @@ describe('ReactCache', () => {
</Suspense>,
);

await waitForAll([1, 'Suspend! [4]', 'Loading...']);

await act(() => jest.advanceTimersByTime(100));
assertLog([
'Promise resolved [4]',

await waitForAll([
1,
4,
'Suspend! [5]',
'Suspend! [4]',
'Loading...',
1,
4,
'Suspend! [4]',
'Suspend! [5]',

'Promise resolved [5]',
1,
4,
5,
]);

await act(() => jest.advanceTimersByTime(100));
assertLog(['Promise resolved [4]', 'Promise resolved [5]', 1, 4, 5]);

expect(root).toMatchRenderedOutput('145');

// We've now rendered values 1, 2, 3, 4, 5, over our limit of 3. The least
Expand All @@ -271,24 +284,14 @@ describe('ReactCache', () => {
// 2 and 3 suspend because they were evicted from the cache
'Suspend! [2]',
'Loading...',
]);

await act(() => jest.advanceTimersByTime(100));
assertLog([
'Promise resolved [2]',

1,
2,
'Suspend! [3]',
1,
2,
'Suspend! [2]',
'Suspend! [3]',

'Promise resolved [3]',
1,
2,
3,
]);

await act(() => jest.advanceTimersByTime(100));
assertLog(['Promise resolved [2]', 'Promise resolved [3]', 1, 2, 3]);
expect(root).toMatchRenderedOutput('123');
});

Expand Down Expand Up @@ -363,7 +366,12 @@ describe('ReactCache', () => {
</Suspense>,
);

await waitForAll(['Suspend! [Hi]', 'Loading...']);
await waitForAll([
'Suspend! [Hi]',
'Loading...',

...(gate('enableSiblingPrerendering') ? ['Suspend! [Hi]'] : []),
]);

resolveThenable('Hi');
// This thenable improperly resolves twice. We should not update the
Expand Down
104 changes: 94 additions & 10 deletions packages/react-devtools-shared/src/__tests__/TimelineProfiler-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,17 @@ import {
normalizeCodeLocInfo,
} from './utils';

import {ReactVersion} from '../../../../ReactVersions';
import semver from 'semver';

// TODO: This is how other DevTools tests access the version but we should find
// a better solution for this
const ReactVersionTestingAgainst = process.env.REACT_VERSION || ReactVersion;
const enableSiblingPrerendering = semver.gte(
ReactVersionTestingAgainst,
'19.0.0',
);

describe('Timeline profiler', () => {
let React;
let Scheduler;
Expand Down Expand Up @@ -1651,7 +1662,11 @@ describe('Timeline profiler', () => {
</React.Suspense>,
);

await waitForAll(['suspended']);
await waitForAll([
'suspended',

...(enableSiblingPrerendering ? ['suspended'] : []),
]);

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

// Verify the Suspense event and duration was recorded.
expect(timelineData.suspenseEvents).toHaveLength(1);
const suspenseEvent = timelineData.suspenseEvents[0];
expect(suspenseEvent).toMatchInlineSnapshot(`
if (enableSiblingPrerendering) {
expect(timelineData.suspenseEvents).toMatchInlineSnapshot(`
[
{
"componentName": "Example",
"depth": 0,
"duration": 10,
"id": "0",
"phase": "mount",
"promiseName": "",
"resolution": "resolved",
"timestamp": 10,
"type": "suspense",
"warning": null,
},
{
"componentName": "Example",
"depth": 0,
"duration": 10,
"id": "0",
"phase": "mount",
"promiseName": "",
"resolution": "resolved",
"timestamp": 10,
"type": "suspense",
"warning": null,
},
]
`);
} else {
const suspenseEvent = timelineData.suspenseEvents[0];
expect(suspenseEvent).toMatchInlineSnapshot(`
{
"componentName": "Example",
"depth": 0,
Expand All @@ -1678,10 +1722,13 @@ describe('Timeline profiler', () => {
"warning": null,
}
`);
}

// There should be two batches of renders: Suspeneded and resolved.
expect(timelineData.batchUIDToMeasuresMap.size).toBe(2);
expect(timelineData.componentMeasures).toHaveLength(2);
expect(timelineData.componentMeasures).toHaveLength(
enableSiblingPrerendering ? 3 : 2,
);
});

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

await waitForAll(['suspended']);
await waitForAll([
'suspended',

...(enableSiblingPrerendering ? ['suspended'] : []),
]);

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

// Verify the Suspense event and duration was recorded.
expect(timelineData.suspenseEvents).toHaveLength(1);
const suspenseEvent = timelineData.suspenseEvents[0];
expect(suspenseEvent).toMatchInlineSnapshot(`
if (enableSiblingPrerendering) {
expect(timelineData.suspenseEvents).toMatchInlineSnapshot(`
[
{
"componentName": "Example",
"depth": 0,
"duration": 10,
"id": "0",
"phase": "mount",
"promiseName": "",
"resolution": "rejected",
"timestamp": 10,
"type": "suspense",
"warning": null,
},
{
"componentName": "Example",
"depth": 0,
"duration": 10,
"id": "0",
"phase": "mount",
"promiseName": "",
"resolution": "rejected",
"timestamp": 10,
"type": "suspense",
"warning": null,
},
]
`);
} else {
expect(timelineData.suspenseEvents).toHaveLength(1);
const suspenseEvent = timelineData.suspenseEvents[0];
expect(suspenseEvent).toMatchInlineSnapshot(`
{
"componentName": "Example",
"depth": 0,
Expand All @@ -1735,10 +1816,13 @@ describe('Timeline profiler', () => {
"warning": null,
}
`);
}

// There should be two batches of renders: Suspeneded and resolved.
expect(timelineData.batchUIDToMeasuresMap.size).toBe(2);
expect(timelineData.componentMeasures).toHaveLength(2);
expect(timelineData.componentMeasures).toHaveLength(
enableSiblingPrerendering ? 3 : 2,
);
});

it('should mark cascading class component state updates', async () => {
Expand Down
14 changes: 12 additions & 2 deletions packages/react-dom/src/__tests__/ReactDOMForm-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1459,13 +1459,23 @@ describe('ReactDOMForm', () => {
</Suspense>,
),
);
assertLog(['Suspend! [Count: 0]', 'Loading...']);
assertLog([
'Suspend! [Count: 0]',
'Loading...',

...(gate('enableSiblingPrerendering') ? ['Suspend! [Count: 0]'] : []),
]);
await act(() => resolveText('Count: 0'));
assertLog(['Count: 0']);

// Dispatch outside of a transition. This will trigger a loading state.
await act(() => dispatch());
assertLog(['Suspend! [Count: 1]', 'Loading...']);
assertLog([
'Suspend! [Count: 1]',
'Loading...',

...(gate('enableSiblingPrerendering') ? ['Suspend! [Count: 1]'] : []),
]);
expect(container.textContent).toBe('Loading...');

await act(() => resolveText('Count: 1'));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,13 @@ describe('ReactDOMSuspensePlaceholder', () => {
});

expect(container.textContent).toEqual('Loading...');
assertLog(['A', 'Suspend! [B]', 'Loading...']);
assertLog([
'A',
'Suspend! [B]',
'Loading...',

...(gate('enableSiblingPrerendering') ? ['A', 'Suspend! [B]', 'C'] : []),
]);
await act(() => {
resolveText('B');
});
Expand Down
Loading

0 comments on commit d6cb4e7

Please sign in to comment.