Clear finished discrete updates during commit phase (#18515)
* Reproduce a bug where `flushDiscreteUpdates` causes fallback never to be committed * Ping suspended level when canceling its timer 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. * Clear finished discrete updates during commit phase If a root is finished at a priority lower than that of the latest pending discrete updates on it, these updates must have been finished so we can clear them now. Otherwise, a later call of `flushDiscreteUpdates` would start a new empty render pass which may cause a scheduled timeout to be cancelled. * Add TODO Happened to find this while writing a test. A JSX element comparison failed because one of them elements had a functional component as an owner, which should ever happen. I'll add a regression test later. Co-authored-by: Andrew Clark <git@andrewclark.io>
jddxf committed
Apr 8, 2020 at 04:34 UTC
ddc4b65cfe17b3f08ff9f18f8804ff5b663788c8
2 files changed
+77
packages/react-reconciler/src/ReactFiberWorkLoop.js
+29
@@ -1168,6 +1168,19 @@ function prepareFreshStack(root, expirationTime) {
1168
cancelTimeout(timeoutHandle);
1169
}
1170
1171
+ // Check if there's a suspended level at lower priority.
1172
+ const lastSuspendedTime = root.lastSuspendedTime;
1173
+ if (lastSuspendedTime !== NoWork && lastSuspendedTime < expirationTime) {
1174
+ const lastPingedTime = root.lastPingedTime;
1175
+ // Make sure the suspended level is marked as pinged so that we return back
1176
+ // to it later, in case the render we're about to start gets aborted.
1177
+ // Generally we only reach this path via a ping, but we shouldn't assume
1178
+ // that will always be the case.
1179
+ if (lastPingedTime === NoWork || lastPingedTime > lastSuspendedTime) {
1180
+ root.lastPingedTime = lastSuspendedTime;
1181
+ }
1182
+ }
1183
+
1184
if (workInProgress !== null) {
1185
let interruptedWork = workInProgress.return;
1186
while (interruptedWork !== null) {
@@ -1202,6 +1215,9 @@ function handleError(root, thrownValue) {
1215
resetContextDependencies();
1216
resetHooksAfterThrow();
1217
resetCurrentDebugFiberInDEV();
1218
+ // TODO: I found and added this missing line while investigating a
1219
+ // separate issue. Write a regression test using string refs.
1220
+ ReactCurrentOwner.current = null;
1221
1222
if (workInProgress === null || workInProgress.return === null) {
1223
// Expected to be working on a non-root fiber. This is a fatal error
@@ -1769,6 +1785,19 @@ function commitRootImpl(root, renderPriorityLevel) {
1785
remainingExpirationTimeBeforeCommit,
1786
);
1787
1788
+ // Clear already finished discrete updates in case that a later call of
1789
+ // `flushDiscreteUpdates` starts a useless render pass which may cancels
1790
+ // a scheduled timeout.
1791
+ if (rootsWithPendingDiscreteUpdates !== null) {
1792
+ const lastDiscreteTime = rootsWithPendingDiscreteUpdates.get(root);
1793
+ if (
1794
+ lastDiscreteTime !== undefined &&
1795
+ remainingExpirationTimeBeforeCommit < lastDiscreteTime
1796
+ ) {
1797
+ rootsWithPendingDiscreteUpdates.delete(root);
1798
+ }
1799
+ }
1800
+
1801
if (root === workInProgressRoot) {
1802
// We can reset these now that they are finished.
1803
workInProgressRoot = null;
packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
+48
@@ -3597,4 +3597,52 @@ describe('ReactSuspenseWithNoopRenderer', () => {
3597
</>,
3598
);
3599
});
3600
+
3601
+ it('regression: empty render at high priority causes update to be dropped', async () => {
3602
+ // Reproduces a bug where flushDiscreteUpdates starts a new (empty) render
3603
+ // pass which cancels a scheduled timeout and causes the fallback never to
3604
+ // be committed.
3605
+ function App({text, shouldSuspend}) {
3606
+ return (
3607
+ <>
3608
+ <Text text={text} />
3609
+ <Suspense fallback={<Text text="Loading..." />}>
3610
+ {shouldSuspend && <AsyncText text="B" />}
3611
+ </Suspense>
3612
+ </>
3613
+ );
3614
+ }
3615
+
3616
+ const root = ReactNoop.createRoot();
3617
+ ReactNoop.discreteUpdates(() => {
3618
+ // High pri
3619
+ root.render(<App text="A" />);
3620
+ });
3621
+ // Low pri
3622
+ root.render(<App text="A" shouldSuspend={true} />);
3623
+
3624
+ expect(Scheduler).toFlushAndYield([
3625
+ // Render the high pri update
3626
+ 'A',
3627
+ // Render the low pri update
3628
+ 'A',
3629
+ 'Suspend! [B]',
3630
+ 'Loading...',
3631
+ ]);
3632
+ expect(root).toMatchRenderedOutput(<span prop="A" />);
3633
+
3634
+ // Triggers erstwhile bug where flushDiscreteUpdates caused an empty render
3635
+ // at a previously committed level
3636
+ ReactNoop.flushDiscreteUpdates();
3637
+
3638
+ // Commit the placeholder
3639
+ Scheduler.unstable_advanceTime(2000);
3640
+ await advanceTimers(2000);
3641
+ expect(root).toMatchRenderedOutput(
3642
+ <>
3643
+ <span prop="A" />
3644
+ <span prop="Loading..." />
3645
+ </>,
3646
+ );
3647
+ });
3648
});