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

More robust fix for #18515 #18535

Merged
merged 2 commits into from Apr 8, 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
19 changes: 8 additions & 11 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Expand Up @@ -679,9 +679,10 @@ function performConcurrentWorkOnRoot(root, didTimeout) {

// We now have a consistent tree. The next step is either to commit it,
// or, if something suspended, wait to commit it after a timeout.
const finishedWork: Fiber = ((root.finishedWork =
root.current.alternate): any);
const finishedWork: Fiber = (root.current.alternate: any);
root.finishedWork = finishedWork;
root.finishedExpirationTime = expirationTime;
root.nextKnownPendingLevel = getRemainingExpirationTime(finishedWork);
finishConcurrentRender(root, finishedWork, exitStatus, expirationTime);
}

Expand Down Expand Up @@ -717,9 +718,6 @@ function finishConcurrentRender(
case RootSuspended: {
markRootSuspendedAtTime(root, expirationTime);
const lastSuspendedTime = root.lastSuspendedTime;
if (expirationTime === lastSuspendedTime) {
root.nextKnownPendingLevel = getRemainingExpirationTime(finishedWork);
}

// We have an acceptable loading state. We need to figure out if we
// should immediately commit it or wait a bit.
Expand Down Expand Up @@ -792,9 +790,6 @@ function finishConcurrentRender(
case RootSuspendedWithDelay: {
markRootSuspendedAtTime(root, expirationTime);
const lastSuspendedTime = root.lastSuspendedTime;
if (expirationTime === lastSuspendedTime) {
root.nextKnownPendingLevel = getRemainingExpirationTime(finishedWork);
}

if (
// do not delay if we're inside an act() scope
Expand Down Expand Up @@ -979,9 +974,10 @@ function performSyncWorkOnRoot(root) {

// We now have a consistent tree. Because this is a sync render, we
// will commit it even if something suspended.
root.finishedWork = (root.current.alternate: any);
const finishedWork: Fiber = (root.current.alternate: any);
root.finishedWork = finishedWork;
root.finishedExpirationTime = expirationTime;

root.nextKnownPendingLevel = getRemainingExpirationTime(finishedWork);
Copy link
Collaborator

@gaearon gaearon Apr 8, 2020

Choose a reason for hiding this comment

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

Is it worth writing a regression test that would fail if this and the defensive safeguard were removed? Looks like the new test covers the Concurrent render only. Not sure if this matters.

Copy link
Collaborator Author

@acdlite acdlite Apr 8, 2020

Choose a reason for hiding this comment

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

The diff view is misleading because of collapsed sections. This line is the sync path. The concurrent path is here: https://github.com/facebook/react/pull/18535/files#diff-63c455475b8566c10993b2daa2c3211bR685

The test in #18515 covers the sync path because the discrete update is expired but I can add another test since that one happens to be fixed by the other change.

commitRoot(root);

// Before exiting, make sure there's a callback scheduled for the next
Expand Down Expand Up @@ -1176,6 +1172,8 @@ function prepareFreshStack(root, expirationTime) {
// 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.
// Note: This is defensive coding to prevent a pending commit from
// being dropped without being rescheduled. It shouldn't be necessary.
if (lastPingedTime === NoWork || lastPingedTime > lastSuspendedTime) {
root.lastPingedTime = lastSuspendedTime;
}
Expand Down Expand Up @@ -1772,7 +1770,6 @@ function commitRootImpl(root, renderPriorityLevel) {
root.callbackNode = null;
root.callbackExpirationTime = NoWork;
root.callbackPriority = NoPriority;
root.nextKnownPendingLevel = NoWork;

// Update the first and last pending times on this root. The new first
// pending time is whatever is left on the root fiber.
Expand Down
Expand Up @@ -3645,4 +3645,117 @@ describe('ReactSuspenseWithNoopRenderer', () => {
</>,
);
});

it('regression: ping at high priority causes update to be dropped', async () => {
const {useState, useTransition} = React;

let setTextA;
function A() {
const [textA, _setTextA] = useState('A');
setTextA = _setTextA;
return (
<Suspense fallback={<Text text="Loading..." />}>
<AsyncText text={textA} />
</Suspense>
);
}

let setTextB;
let startTransition;
function B() {
const [textB, _setTextB] = useState('B');
const [_startTransition] = useTransition({timeoutMs: 10000});
startTransition = _startTransition;
setTextB = _setTextB;
return (
<Suspense fallback={<Text text="Loading..." />}>
<AsyncText text={textB} />
</Suspense>
);
}

function App() {
return (
<>
<A />
<B />
</>
);
}

const root = ReactNoop.createRoot();
await ReactNoop.act(async () => {
await resolveText('A');
await resolveText('B');
root.render(<App />);
});
expect(Scheduler).toHaveYielded(['A', 'B']);
expect(root).toMatchRenderedOutput(
<>
<span prop="A" />
<span prop="B" />
</>,
);

await ReactNoop.act(async () => {
// Triggers suspense at normal pri
setTextA('A1');
// Triggers in an unrelated tree at a different pri
startTransition(() => {
// Update A again so that it doesn't suspend on A1. That way we can ping
// the A1 update without also pinging this one. This is a workaround
// because there's currently no way to render at a lower priority (B2)
// without including all updates at higher priority (A1).
setTextA('A2');
setTextB('B2');
});
});
expect(Scheduler).toHaveYielded([
'B',
'Suspend! [A1]',
'Loading...',

'Suspend! [A2]',
'Loading...',
'Suspend! [B2]',
'Loading...',
]);
expect(root).toMatchRenderedOutput(
<>
<span prop="A" />
<span prop="B" />
</>,
);

await ReactNoop.act(async () => {
resolveText('A1');
});
expect(Scheduler).toHaveYielded([
'Promise resolved [A1]',
'A1',
'Suspend! [A2]',
'Loading...',
'Suspend! [B2]',
'Loading...',
]);
expect(root).toMatchRenderedOutput(
<>
<span prop="A1" />
<span prop="B" />
</>,
);

// Commit the placeholder
Scheduler.unstable_advanceTime(20000);
await advanceTimers(20000);

expect(root).toMatchRenderedOutput(
<>
<span hidden={true} prop="A1" />
<span prop="Loading..." />
<span hidden={true} prop="B" />
<span prop="Loading..." />
</>,
);
});
});