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

Reset lastEffect when resuming SuspenseList #18412

Merged
merged 1 commit into from Mar 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/react-reconciler/src/ReactFiberBeginWork.js
Expand Up @@ -3153,6 +3153,7 @@ function beginWork(
// update in the past but didn't complete it.
renderState.rendering = null;
renderState.tail = null;
renderState.lastEffect = null;
}
pushSuspenseContext(workInProgress, suspenseStackCursor.current);

Expand Down
Expand Up @@ -2300,4 +2300,189 @@ describe('ReactSuspenseList', () => {
// remount.
expect(previousInst).toBe(setAsyncB);
});

it('is able to re-suspend the last rows during an update with hidden', async () => {
let AsyncB = createAsyncText('B');

let setAsyncB;

function B() {
let [shouldBeAsync, setAsync] = React.useState(false);
setAsyncB = setAsync;

return shouldBeAsync ? (
<Suspense fallback={<Text text="Loading B" />}>
<AsyncB />
</Suspense>
) : (
<Text text="Sync B" />
);
}

function Foo({updateList}) {
return (
<SuspenseList revealOrder="forwards" tail="hidden">
<Suspense key="A" fallback={<Text text="Loading A" />}>
<Text text="A" />
</Suspense>
<B key="B" updateList={updateList} />
</SuspenseList>
);
}

ReactNoop.render(<Foo />);

expect(Scheduler).toFlushAndYield(['A', 'Sync B']);

expect(ReactNoop).toMatchRenderedOutput(
<>
<span>A</span>
<span>Sync B</span>
</>,
);

let previousInst = setAsyncB;

// During an update we suspend on B.
ReactNoop.act(() => setAsyncB(true));

expect(Scheduler).toHaveYielded([
'Suspend! [B]',
'Loading B',
// The second pass is the "force hide" pass
'Loading B',
]);

expect(ReactNoop).toMatchRenderedOutput(
<>
<span>A</span>
<span>Loading B</span>
</>,
);

// Before we resolve we'll rerender the whole list.
// This should leave the tree intact.
ReactNoop.act(() => ReactNoop.render(<Foo updateList={true} />));

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

expect(ReactNoop).toMatchRenderedOutput(
<>
<span>A</span>
<span>Loading B</span>
</>,
);

await AsyncB.resolve();

expect(Scheduler).toFlushAndYield(['B']);

expect(ReactNoop).toMatchRenderedOutput(
<>
<span>A</span>
<span>B</span>
</>,
);

// This should be the same instance. I.e. it didn't
// remount.
expect(previousInst).toBe(setAsyncB);
});

it('is able to interrupt a partially rendered tree and continue later', async () => {
let AsyncA = createAsyncText('A');

let updateLowPri;
let updateHighPri;

function Bar() {
let [highPriState, setHighPriState] = React.useState(false);
updateHighPri = setHighPriState;
return highPriState ? <AsyncA /> : null;
}

function Foo() {
let [lowPriState, setLowPriState] = React.useState(false);
updateLowPri = setLowPriState;
return (
<SuspenseList revealOrder="forwards" tail="hidden">
<Suspense key="A" fallback={<Text text="Loading A" />}>
<Bar />
</Suspense>
{lowPriState ? <Text text="B" /> : null}
{lowPriState ? <Text text="C" /> : null}
{lowPriState ? <Text text="D" /> : null}
</SuspenseList>
);
}

ReactNoop.render(<Foo />);

expect(Scheduler).toFlushAndYield([]);

expect(ReactNoop).toMatchRenderedOutput(null);

ReactNoop.act(() => {
// Add a few items at the end.
updateLowPri(true);

// Flush partially through.
expect(Scheduler).toFlushAndYieldThrough(['B', 'C']);

// Schedule another update at higher priority.
Scheduler.unstable_runWithPriority(
Scheduler.unstable_UserBlockingPriority,
() => updateHighPri(true),
);

// That will intercept the previous render.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interrupt? :-)

});

jest.runAllTimers();

expect(Scheduler).toHaveYielded(
__DEV__
? [
// First attempt at high pri.
'Suspend! [A]',
'Loading A',
// Re-render at forced.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "forced" mean here? Which branch does it correspond to? Explain like I'm five.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SuspenseList sets the suspense context that during render “forces” the suspense boundary into fallback state.

In this case it’s only one boundary so it doesn’t need to but we don’t know. If there was another boundary in the row we would have to rerender that one so that’s the second pass.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh that makes sense! I didn’t realize it works this way.

'Suspend! [A]',
'Loading A',
// We auto-commit this on DEV.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? "Flush fallbacks" flag?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea

// Try again on low-pri.
'Suspend! [A]',
'Loading A',
]
: [
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test is verifying the same logs appear three or four times. But how do we know the comments are actually telling the truth? Can we include the boolean values in logs so that we know we're actually rendering at the priority suggested by comments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d rather just delete this assertion. It’s unrelated. It’s just that Scheduler expects force me to assert. This is super suboptimal so it’ll change by heuristics a lot.

// First attempt at high pri.
'Suspend! [A]',
'Loading A',
// Re-render at forced.
'Suspend! [A]',
'Loading A',
// We didn't commit so retry at low-pri.
'Suspend! [A]',
'Loading A',
// Re-render at forced.
'Suspend! [A]',
'Loading A',
],
);

expect(ReactNoop).toMatchRenderedOutput(<span>Loading A</span>);

await AsyncA.resolve();

expect(Scheduler).toFlushAndYield(['A', 'B', 'C', 'D']);

expect(ReactNoop).toMatchRenderedOutput(
<>
<span>A</span>
<span>B</span>
<span>C</span>
<span>D</span>
</>,
);
});
});