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.

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

[Suspense] Change Suspending and Restarting Heuristics #15769

Merged
merged 13 commits into from
May 30, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Prev Previous commit
Next Next commit
Adjust Suspense tests to account for new heuristics
Mostly this just means render the Suspense boundary first so that it
becomes an update instead of initial mount.
  • Loading branch information
sebmarkbage committed May 30, 2019
commit 6a3dde2a6f7f37ae1974f1145ddff1936d2ed9cb
Original file line number Diff line number Diff line change
Expand Up @@ -98,22 +98,32 @@ describe('ReactSuspense', () => {
return props.children;
}

function Foo() {
function Foo({renderBar}) {
Scheduler.yieldValue('Foo');
return (
<Suspense fallback={<Text text="Loading..." />}>
<Bar>
<AsyncText text="A" ms={100} />
<Text text="B" />
</Bar>
{renderBar ? (
<Bar>
<AsyncText text="A" ms={100} />
<Text text="B" />
</Bar>
) : null}
</Suspense>
);
}

// Render an empty shell
const root = ReactTestRenderer.create(<Foo />, {
unstable_isConcurrent: true,
});

expect(Scheduler).toFlushAndYield(['Foo']);
expect(root).toMatchRenderedOutput(null);

// Navigate the shell to now render the child content.
// This should suspend.
root.update(<Foo renderBar={true} />);

expect(Scheduler).toFlushAndYield([
'Foo',
'Bar',
Expand Down Expand Up @@ -161,17 +171,12 @@ describe('ReactSuspense', () => {
'Suspend! [B]',
'Loading B...',
]);
expect(root).toMatchRenderedOutput(null);

// Advance time by enough to timeout both components and commit their placeholders
jest.advanceTimersByTime(4000);
expect(Scheduler).toFlushWithoutYielding();
expect(root).toMatchRenderedOutput('Loading A...Loading B...');

// Advance time by enough that the first Suspense's promise resolves and
// switches back to the normal view. The second Suspense should still
// show the placeholder
jest.advanceTimersByTime(1000);
jest.advanceTimersByTime(5000);
// TODO: Should we throw if you forget to call toHaveYielded?
expect(Scheduler).toHaveYielded(['Promise resolved [A]']);
expect(Scheduler).toFlushAndYield(['A']);
Expand Down Expand Up @@ -214,28 +219,48 @@ describe('ReactSuspense', () => {
}

const root = ReactTestRenderer.create(
<Suspense fallback={<Text text="Loading..." />}>
<Async />
<Text text="Sibling" />
</Suspense>,
<React.Fragment>
<Suspense fallback={<Text text="Loading..." />} />
<Text text="Initial" />
</React.Fragment>,
{
unstable_isConcurrent: true,
},
);
expect(Scheduler).toFlushAndYield(['Initial']);
expect(root).toMatchRenderedOutput('Initial');

expect(Scheduler).toFlushAndYieldThrough(['Suspend!']);
// The update will suspend.
root.update(
<React.Fragment>
<Suspense fallback={<Text text="Loading..." />}>
<Async />
</Suspense>
<Text text="After Suspense" />
<Text text="Sibling" />
</React.Fragment>,
);

// Yield past the Suspense boundary but don't complete the last sibling.
expect(Scheduler).toFlushAndYieldThrough([
'Suspend!',
'Loading...',
'After Suspense',
]);

// The promise resolves before the current render phase has completed
resolveThenable();
expect(Scheduler).toHaveYielded([]);
expect(root).toMatchRenderedOutput('Initial');

// Start over from the root, instead of continuing.
expect(Scheduler).toFlushAndYield([
// Async renders again *before* Sibling
'Async',
'After Suspense',
'Sibling',
]);
expect(root).toMatchRenderedOutput('AsyncSibling');
expect(root).toMatchRenderedOutput('AsyncAfter SuspenseSibling');
});

it('mounts a lazy class component in non-concurrent mode', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ describe('ReactSuspensePlaceholder', () => {
ReactNoop.render(<App middleText="B" />);

expect(Scheduler).toFlushAndYield(['A', 'Suspend! [B]', 'C', 'Loading...']);
expect(ReactNoop).toMatchRenderedOutput(null);
expect(ReactNoop).toMatchRenderedOutput('Loading...');

jest.advanceTimersByTime(1000);
expect(Scheduler).toHaveYielded(['Promise resolved [B]']);
Expand Down Expand Up @@ -189,7 +189,7 @@ describe('ReactSuspensePlaceholder', () => {

expect(Scheduler).toFlushAndYield(['A', 'Suspend! [B]', 'C', 'Loading...']);

expect(ReactNoop).toMatchRenderedOutput(null);
expect(ReactNoop).not.toMatchRenderedOutput('ABC');

jest.advanceTimersByTime(1000);
expect(Scheduler).toHaveYielded(['Promise resolved [B]']);
Expand Down Expand Up @@ -239,7 +239,7 @@ describe('ReactSuspensePlaceholder', () => {

expect(Scheduler).toFlushAndYield(['a', 'Suspend! [b]', 'c', 'Loading...']);

expect(ReactNoop).toMatchRenderedOutput(null);
expect(ReactNoop).toMatchRenderedOutput(<uppercase>LOADING...</uppercase>);

jest.advanceTimersByTime(1000);
expect(Scheduler).toHaveYielded(['Promise resolved [b]']);
Expand Down Expand Up @@ -345,10 +345,8 @@ describe('ReactSuspensePlaceholder', () => {
'Text',
'Fallback',
]);
expect(ReactNoop).toMatchRenderedOutput(null);

// Show the fallback UI.
jest.advanceTimersByTime(750);
// Since this is initial render we immediately commit the fallback. Another test below
// deals with the update case where this suspends.
expect(ReactNoop).toMatchRenderedOutput('Loading...');
expect(onRender).toHaveBeenCalledTimes(1);

Expand All @@ -359,7 +357,7 @@ describe('ReactSuspensePlaceholder', () => {
expect(onRender.mock.calls[0][3]).toBe(10);

// Resolve the pending promise.
jest.advanceTimersByTime(250);
jest.advanceTimersByTime(1000);
expect(Scheduler).toHaveYielded(['Promise resolved [Loaded]']);
expect(Scheduler).toFlushAndYield(['Suspending', 'Loaded', 'Text']);
expect(ReactNoop).toMatchRenderedOutput('LoadedText');
Expand Down Expand Up @@ -437,7 +435,12 @@ describe('ReactSuspensePlaceholder', () => {
});

it('properly accounts for base durations when a suspended times out in a concurrent tree', () => {
ReactNoop.render(<App shouldSuspend={false} textRenderDuration={5} />);
ReactNoop.render(
<React.Fragment>
<App shouldSuspend={false} textRenderDuration={5} />
<Suspense fallback={null} />
</React.Fragment>,
);

expect(Scheduler).toFlushAndYield(['App', 'Text']);
expect(ReactNoop).toMatchRenderedOutput('Text');
Expand All @@ -448,7 +451,12 @@ describe('ReactSuspensePlaceholder', () => {
expect(onRender.mock.calls[0][2]).toBe(5);
expect(onRender.mock.calls[0][3]).toBe(5);

ReactNoop.render(<App shouldSuspend={true} textRenderDuration={5} />);
ReactNoop.render(
<React.Fragment>
<App shouldSuspend={true} textRenderDuration={5} />
<Suspense fallback={null} />
</React.Fragment>,
);
expect(Scheduler).toFlushAndYield([
'App',
'Suspending',
Expand Down
Loading