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

Clear finished discrete updates during commit phase #18515

Merged
merged 4 commits into from Apr 7, 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
29 changes: 29 additions & 0 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Expand Up @@ -1168,6 +1168,19 @@ function prepareFreshStack(root, expirationTime) {
cancelTimeout(timeoutHandle);
}

// Check if there's a suspended level at lower priority.
const lastSuspendedTime = root.lastSuspendedTime;
if (lastSuspendedTime !== NoWork && lastSuspendedTime < expirationTime) {
const lastPingedTime = root.lastPingedTime;
// Make sure the suspended level is marked as pinged so that we return back
// to it later, in case the render we're about to start gets aborted.
// Generally we only reach this path via a ping, but we shouldn't assume
// that will always be the case.
if (lastPingedTime === NoWork || lastPingedTime > lastSuspendedTime) {
root.lastPingedTime = lastSuspendedTime;
}
}

if (workInProgress !== null) {
let interruptedWork = workInProgress.return;
while (interruptedWork !== null) {
Expand Down Expand Up @@ -1202,6 +1215,9 @@ function handleError(root, thrownValue) {
resetContextDependencies();
resetHooksAfterThrow();
resetCurrentDebugFiberInDEV();
// TODO: I found and added this missing line while investigating a
// separate issue. Write a regression test using string refs.
ReactCurrentOwner.current = null;

if (workInProgress === null || workInProgress.return === null) {
// Expected to be working on a non-root fiber. This is a fatal error
Expand Down Expand Up @@ -1769,6 +1785,19 @@ function commitRootImpl(root, renderPriorityLevel) {
remainingExpirationTimeBeforeCommit,
);

// Clear already finished discrete updates in case that a later call of
// `flushDiscreteUpdates` starts a useless render pass which may cancels
// a scheduled timeout.
if (rootsWithPendingDiscreteUpdates !== null) {
const lastDiscreteTime = rootsWithPendingDiscreteUpdates.get(root);
if (
lastDiscreteTime !== undefined &&
remainingExpirationTimeBeforeCommit < lastDiscreteTime
) {
rootsWithPendingDiscreteUpdates.delete(root);
}
}

if (root === workInProgressRoot) {
// We can reset these now that they are finished.
workInProgressRoot = null;
Expand Down
Expand Up @@ -3597,4 +3597,52 @@ describe('ReactSuspenseWithNoopRenderer', () => {
</>,
);
});

it('regression: empty render at high priority causes update to be dropped', async () => {
// Reproduces a bug where flushDiscreteUpdates starts a new (empty) render
// pass which cancels a scheduled timeout and causes the fallback never to
// be committed.
function App({text, shouldSuspend}) {
return (
<>
<Text text={text} />
<Suspense fallback={<Text text="Loading..." />}>
{shouldSuspend && <AsyncText text="B" />}
</Suspense>
</>
);
}

const root = ReactNoop.createRoot();
ReactNoop.discreteUpdates(() => {
// High pri
root.render(<App text="A" />);
});
// Low pri
root.render(<App text="A" shouldSuspend={true} />);

expect(Scheduler).toFlushAndYield([
// Render the high pri update
'A',
// Render the low pri update
'A',
'Suspend! [B]',
'Loading...',
]);
expect(root).toMatchRenderedOutput(<span prop="A" />);

// Triggers erstwhile bug where flushDiscreteUpdates caused an empty render
// at a previously committed level
ReactNoop.flushDiscreteUpdates();

// Commit the placeholder
Scheduler.unstable_advanceTime(2000);
await advanceTimers(2000);
expect(root).toMatchRenderedOutput(
<>
<span prop="A" />
<span prop="Loading..." />
</>,
);
});
});