More robust fix for #18515 (#18535)
* Add another test for #18515 using pings Adds a regression test for the same underlying bug as #18515 but using pings. Test already passes, but I confirmed it fails if you revert the fix in #18515. * Set nextPendingLevel after commit, too
Andrew Clark committed
Apr 7, 2020 at 18:52 UTC
2def7b3caf4b15ade69eb16b5ec87afa0dcd7992
2 files changed
+121
-11
packages/react-reconciler/src/ReactFiberWorkLoop.js
+8
-11
@@ -679,9 +679,10 @@ function performConcurrentWorkOnRoot(root, didTimeout) {
679
680
// We now have a consistent tree. The next step is either to commit it,
681
// or, if something suspended, wait to commit it after a timeout.
682
- const finishedWork: Fiber = ((root.finishedWork =
683
- root.current.alternate): any);
682
+ const finishedWork: Fiber = (root.current.alternate: any);
683
+ root.finishedWork = finishedWork;
684
root.finishedExpirationTime = expirationTime;
685
+ root.nextKnownPendingLevel = getRemainingExpirationTime(finishedWork);
686
finishConcurrentRender(root, finishedWork, exitStatus, expirationTime);
687
}
688
@@ -717,9 +718,6 @@ function finishConcurrentRender(
718
case RootSuspended: {
719
markRootSuspendedAtTime(root, expirationTime);
720
const lastSuspendedTime = root.lastSuspendedTime;
720
- if (expirationTime === lastSuspendedTime) {
721
- root.nextKnownPendingLevel = getRemainingExpirationTime(finishedWork);
722
- }
721
722
// We have an acceptable loading state. We need to figure out if we
723
// should immediately commit it or wait a bit.
@@ -792,9 +790,6 @@ function finishConcurrentRender(
790
case RootSuspendedWithDelay: {
791
markRootSuspendedAtTime(root, expirationTime);
792
const lastSuspendedTime = root.lastSuspendedTime;
795
- if (expirationTime === lastSuspendedTime) {
796
- root.nextKnownPendingLevel = getRemainingExpirationTime(finishedWork);
797
- }
793
794
if (
795
// do not delay if we're inside an act() scope
@@ -979,9 +974,10 @@ function performSyncWorkOnRoot(root) {
974
975
// We now have a consistent tree. Because this is a sync render, we
976
// will commit it even if something suspended.
982
- root.finishedWork = (root.current.alternate: any);
977
+ const finishedWork: Fiber = (root.current.alternate: any);
978
+ root.finishedWork = finishedWork;
979
root.finishedExpirationTime = expirationTime;
984
-
980
+ root.nextKnownPendingLevel = getRemainingExpirationTime(finishedWork);
981
commitRoot(root);
982
983
// Before exiting, make sure there's a callback scheduled for the next
@@ -1176,6 +1172,8 @@ function prepareFreshStack(root, expirationTime) {
1172
// to it later, in case the render we're about to start gets aborted.
1173
// Generally we only reach this path via a ping, but we shouldn't assume
1174
// that will always be the case.
1175
+ // Note: This is defensive coding to prevent a pending commit from
1176
+ // being dropped without being rescheduled. It shouldn't be necessary.
1177
if (lastPingedTime === NoWork || lastPingedTime > lastSuspendedTime) {
1178
root.lastPingedTime = lastSuspendedTime;
1179
}
@@ -1772,7 +1770,6 @@ function commitRootImpl(root, renderPriorityLevel) {
1770
root.callbackNode = null;
1771
root.callbackExpirationTime = NoWork;
1772
root.callbackPriority = NoPriority;
1775
- root.nextKnownPendingLevel = NoWork;
1773
1774
// Update the first and last pending times on this root. The new first
1775
// pending time is whatever is left on the root fiber.
packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
+113
@@ -3645,4 +3645,117 @@ describe('ReactSuspenseWithNoopRenderer', () => {
3645
</>,
3646
);
3647
});
3648
+
3649
+ it('regression: ping at high priority causes update to be dropped', async () => {
3650
+ const {useState, useTransition} = React;
3651
+
3652
+ let setTextA;
3653
+ function A() {
3654
+ const [textA, _setTextA] = useState('A');
3655
+ setTextA = _setTextA;
3656
+ return (
3657
+ <Suspense fallback={<Text text="Loading..." />}>
3658
+ <AsyncText text={textA} />
3659
+ </Suspense>
3660
+ );
3661
+ }
3662
+
3663
+ let setTextB;
3664
+ let startTransition;
3665
+ function B() {
3666
+ const [textB, _setTextB] = useState('B');
3667
+ const [_startTransition] = useTransition({timeoutMs: 10000});
3668
+ startTransition = _startTransition;
3669
+ setTextB = _setTextB;
3670
+ return (
3671
+ <Suspense fallback={<Text text="Loading..." />}>
3672
+ <AsyncText text={textB} />
3673
+ </Suspense>
3674
+ );
3675
+ }
3676
+
3677
+ function App() {
3678
+ return (
3679
+ <>
3680
+ <A />
3681
+ <B />
3682
+ </>
3683
+ );
3684
+ }
3685
+
3686
+ const root = ReactNoop.createRoot();
3687
+ await ReactNoop.act(async () => {
3688
+ await resolveText('A');
3689
+ await resolveText('B');
3690
+ root.render(<App />);
3691
+ });
3692
+ expect(Scheduler).toHaveYielded(['A', 'B']);
3693
+ expect(root).toMatchRenderedOutput(
3694
+ <>
3695
+ <span prop="A" />
3696
+ <span prop="B" />
3697
+ </>,
3698
+ );
3699
+
3700
+ await ReactNoop.act(async () => {
3701
+ // Triggers suspense at normal pri
3702
+ setTextA('A1');
3703
+ // Triggers in an unrelated tree at a different pri
3704
+ startTransition(() => {
3705
+ // Update A again so that it doesn't suspend on A1. That way we can ping
3706
+ // the A1 update without also pinging this one. This is a workaround
3707
+ // because there's currently no way to render at a lower priority (B2)
3708
+ // without including all updates at higher priority (A1).
3709
+ setTextA('A2');
3710
+ setTextB('B2');
3711
+ });
3712
+ });
3713
+ expect(Scheduler).toHaveYielded([
3714
+ 'B',
3715
+ 'Suspend! [A1]',
3716
+ 'Loading...',
3717
+
3718
+ 'Suspend! [A2]',
3719
+ 'Loading...',
3720
+ 'Suspend! [B2]',
3721
+ 'Loading...',
3722
+ ]);
3723
+ expect(root).toMatchRenderedOutput(
3724
+ <>
3725
+ <span prop="A" />
3726
+ <span prop="B" />
3727
+ </>,
3728
+ );
3729
+
3730
+ await ReactNoop.act(async () => {
3731
+ resolveText('A1');
3732
+ });
3733
+ expect(Scheduler).toHaveYielded([
3734
+ 'Promise resolved [A1]',
3735
+ 'A1',
3736
+ 'Suspend! [A2]',
3737
+ 'Loading...',
3738
+ 'Suspend! [B2]',
3739
+ 'Loading...',
3740
+ ]);
3741
+ expect(root).toMatchRenderedOutput(
3742
+ <>
3743
+ <span prop="A1" />
3744
+ <span prop="B" />
3745
+ </>,
3746
+ );
3747
+
3748
+ // Commit the placeholder
3749
+ Scheduler.unstable_advanceTime(20000);
3750
+ await advanceTimers(20000);
3751
+
3752
+ expect(root).toMatchRenderedOutput(
3753
+ <>
3754
+ <span hidden={true} prop="A1" />
3755
+ <span prop="Loading..." />
3756
+ <span hidden={true} prop="B" />
3757
+ <span prop="Loading..." />
3758
+ </>,
3759
+ );
3760
+ });
3761
});