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

Start prerendering Suspense retries immediately #30934

Merged
merged 5 commits into from
Sep 11, 2024
Merged

Start prerendering Suspense retries immediately #30934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
7086f1f
Refine tag before checking StoreConsistency flag
acdlite Sep 5, 2024
1a21c9d
Don't prerender siblings after something throws
acdlite Sep 6, 2024
725d1b0
Don't skip siblings when rendering hidden content
acdlite Sep 8, 2024
dc3ccc2
Start prerendering Suspense retries immediately
acdlite Sep 8, 2024
842f2cf
Prerendering should not cancel a pending commit
acdlite Sep 10, 2024
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
98 changes: 53 additions & 45 deletions packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
8 changes: 7 additions & 1 deletion packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js
View file
Open in desktop
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
Loading