Bugfix: Render phase update causes remaining updates in same component to be dropped (#18537)
* Bugfix: Render phase update leads to dropped work Render phase updates should not affect the `fiber.expirationTime` field. We don't have to set anything on the fiber because we're going to process the render phase update immediately. We also shouldn't reset the `expirationTime` field in between render passes because it represents the remaining work left in the update queues. During the re-render, the updates that were skipped in the original pass are not processed again. I think my original motivation for using this field for render phase updates was so I didn't have to add another module level variable. * Add repro case for #18486 Co-authored-by: Dan Abramov <dan.abramov@me.com>
Andrew Clark committed
Apr 7, 2020 at 19:17 UTC
2dddd1e00cdadad1783aa55ac65a28634590e9c1
3 files changed
+161
-7
packages/react-reconciler/src/ReactFiberHooks.js
+11
-7
@@ -230,6 +230,11 @@ let workInProgressHook: Hook | null = null;
230
// finished evaluating this component. This is an optimization so we know
231
// whether we need to clear render phase updates after a throw.
232
let didScheduleRenderPhaseUpdate: boolean = false;
233
+// Where an update was scheduled only during the current render pass. This
234
+// gets reset after each attempt.
235
+// TODO: Maybe there's some way to consolidate this with
236
+// `didScheduleRenderPhaseUpdate`. Or with `numberOfReRenders`.
237
+let didScheduleRenderPhaseUpdateDuringThisPass: boolean = false;
238
239
const RE_RENDER_LIMIT = 25;
240
@@ -455,13 +460,12 @@ export function renderWithHooks<Props, SecondArg>(
460
let children = Component(props, secondArg);
461
462
// Check if there was a render phase update
458
- if (workInProgress.expirationTime === renderExpirationTime) {
463
+ if (didScheduleRenderPhaseUpdateDuringThisPass) {
464
// Keep rendering in a loop for as long as render phase updates continue to
465
// be scheduled. Use a counter to prevent infinite loops.
466
let numberOfReRenders: number = 0;
467
do {
463
- workInProgress.expirationTime = NoWork;
464
-
468
+ didScheduleRenderPhaseUpdateDuringThisPass = false;
469
invariant(
470
numberOfReRenders < RE_RENDER_LIMIT,
471
'Too many re-renders. React limits the number of renders to prevent ' +
@@ -491,7 +495,7 @@ export function renderWithHooks<Props, SecondArg>(
495
: HooksDispatcherOnRerender;
496
497
children = Component(props, secondArg);
494
- } while (workInProgress.expirationTime === renderExpirationTime);
498
+ } while (didScheduleRenderPhaseUpdateDuringThisPass);
499
}
500
501
// We can assume the previous dispatcher is always this one, since we set it
@@ -564,6 +568,7 @@ export function resetHooksAfterThrow(): void {
568
}
569
hook = hook.next;
570
}
571
+ didScheduleRenderPhaseUpdate = false;
572
}
573
574
renderExpirationTime = NoWork;
@@ -581,7 +586,7 @@ export function resetHooksAfterThrow(): void {
586
isUpdatingOpaqueValueInRenderPhase = false;
587
}
588
584
- didScheduleRenderPhaseUpdate = false;
589
+ didScheduleRenderPhaseUpdateDuringThisPass = false;
590
}
591
592
function mountWorkInProgressHook(): Hook {
@@ -1726,9 +1731,8 @@ function dispatchAction<S, A>(
1731
// This is a render phase update. Stash it in a lazily-created map of
1732
// queue -> linked list of updates. After this render pass, we'll restart
1733
// and apply the stashed updates on top of the work-in-progress hook.
1729
- didScheduleRenderPhaseUpdate = true;
1734
+ didScheduleRenderPhaseUpdateDuringThisPass = didScheduleRenderPhaseUpdate = true;
1735
update.expirationTime = renderExpirationTime;
1731
- currentlyRenderingFiber.expirationTime = renderExpirationTime;
1736
} else {
1737
if (
1738
fiber.expirationTime === NoWork &&
packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
+35
@@ -750,6 +750,41 @@ describe('ReactHooksWithNoopRenderer', () => {
750
expect(root).toMatchRenderedOutput(<span prop="B:0" />);
751
});
752
753
+ it('regression: render phase updates cause lower pri work to be dropped', async () => {
754
+ let setRow;
755
+ function ScrollView() {
756
+ const [row, _setRow] = useState(10);
757
+ setRow = _setRow;
758
+
759
+ const [scrollDirection, setScrollDirection] = useState('Up');
760
+ const [prevRow, setPrevRow] = useState(null);
761
+
762
+ if (prevRow !== row) {
763
+ setScrollDirection(prevRow !== null && row > prevRow ? 'Down' : 'Up');
764
+ setPrevRow(row);
765
+ }
766
+
767
+ return <Text text={scrollDirection} />;
768
+ }
769
+
770
+ const root = ReactNoop.createRoot();
771
+
772
+ await act(async () => {
773
+ root.render(<ScrollView row={10} />);
774
+ });
775
+ expect(Scheduler).toHaveYielded(['Up']);
776
+ expect(root).toMatchRenderedOutput(<span prop="Up" />);
777
+
778
+ await act(async () => {
779
+ ReactNoop.discreteUpdates(() => {
780
+ setRow(5);
781
+ });
782
+ setRow(20);
783
+ });
784
+ expect(Scheduler).toHaveYielded(['Up', 'Down']);
785
+ expect(root).toMatchRenderedOutput(<span prop="Down" />);
786
+ });
787
+
788
// TODO: This should probably warn
789
it.experimental('calling startTransition inside render phase', async () => {
790
let startTransition;
packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
+115
@@ -3758,4 +3758,119 @@ describe('ReactSuspenseWithNoopRenderer', () => {
3758
</>,
3759
);
3760
});
3761
+
3762
+ // Regression: https://github.com/facebook/react/issues/18486
3763
+ it.experimental(
3764
+ 'does not get stuck in pending state with render phase updates',
3765
+ async () => {
3766
+ let setTextWithTransition;
3767
+
3768
+ function App() {
3769
+ const [startTransition, isPending] = React.useTransition({
3770
+ timeoutMs: 30000,
3771
+ });
3772
+ const [text, setText] = React.useState('');
3773
+ const [mirror, setMirror] = React.useState('');
3774
+
3775
+ if (text !== mirror) {
3776
+ // Render phase update was needed to repro the bug.
3777
+ setMirror(text);
3778
+ }
3779
+
3780
+ setTextWithTransition = value => {
3781
+ startTransition(() => {
3782
+ setText(value);
3783
+ });
3784
+ };
3785
+
3786
+ return (
3787
+ <>
3788
+ {isPending ? <Text text="Pending..." /> : null}
3789
+ {text !== '' ? <AsyncText text={text} /> : <Text text={text} />}
3790
+ </>
3791
+ );
3792
+ }
3793
+
3794
+ function Root() {
3795
+ return (
3796
+ <Suspense fallback={<Text text="Loading..." />}>
3797
+ <App />
3798
+ </Suspense>
3799
+ );
3800
+ }
3801
+
3802
+ const root = ReactNoop.createRoot();
3803
+ await ReactNoop.act(async () => {
3804
+ root.render(<Root />);
3805
+ });
3806
+ expect(Scheduler).toHaveYielded(['']);
3807
+ expect(root).toMatchRenderedOutput(<span prop="" />);
3808
+
3809
+ // Update to "a". That will suspend.
3810
+ await ReactNoop.act(async () => {
3811
+ setTextWithTransition('a');
3812
+ // Let it expire. This is important for the repro.
3813
+ Scheduler.unstable_advanceTime(1000);
3814
+ expect(Scheduler).toFlushAndYield([
3815
+ 'Pending...',
3816
+ '',
3817
+ 'Suspend! [a]',
3818
+ 'Loading...',
3819
+ ]);
3820
+ });
3821
+ expect(Scheduler).toHaveYielded([]);
3822
+ expect(root).toMatchRenderedOutput(
3823
+ <>
3824
+ <span prop="Pending..." />
3825
+ <span prop="" />
3826
+ </>,
3827
+ );
3828
+
3829
+ // Update to "b". That will suspend, too.
3830
+ await ReactNoop.act(async () => {
3831
+ setTextWithTransition('b');
3832
+ expect(Scheduler).toFlushAndYield([
3833
+ // Neither is resolved yet.
3834
+ 'Pending...',
3835
+ 'Suspend! [a]',
3836
+ 'Loading...',
3837
+ 'Suspend! [b]',
3838
+ 'Loading...',
3839
+ ]);
3840
+ });
3841
+ expect(Scheduler).toHaveYielded([]);
3842
+ expect(root).toMatchRenderedOutput(
3843
+ <>
3844
+ <span prop="Pending..." />
3845
+ <span prop="" />
3846
+ </>,
3847
+ );
3848
+
3849
+ // Resolve "a". But "b" is still pending.
3850
+ await ReactNoop.act(async () => {
3851
+ await resolveText('a');
3852
+ });
3853
+ expect(Scheduler).toHaveYielded([
3854
+ 'Promise resolved [a]',
3855
+ 'Pending...',
3856
+ 'a',
3857
+ 'Suspend! [b]',
3858
+ 'Loading...',
3859
+ ]);
3860
+ expect(root).toMatchRenderedOutput(
3861
+ <>
3862
+ <span prop="Pending..." />
3863
+ <span prop="a" />
3864
+ </>,
3865
+ );
3866
+
3867
+ // Resolve "b". This should remove the pending state.
3868
+ await ReactNoop.act(async () => {
3869
+ await resolveText('b');
3870
+ });
3871
+ expect(Scheduler).toHaveYielded(['Promise resolved [b]', 'b']);
3872
+ // The bug was that the pending state got stuck forever.
3873
+ expect(root).toMatchRenderedOutput(<span prop="b" />);
3874
+ },
3875
+ );
3876
});