From 36bf07f4307234f7ace90f8bca5d198e00e06b04 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 29 Jan 2020 11:24:43 -0800 Subject: [PATCH] Failing test: Expiring a partially completed tree We should not throw out a partially completed tree if it expires in the middle of rendering. We should finish the rest of the tree without yielding, then finish any remaining expired levels in a single batch. --- .../ReactIncrementalUpdates-test.internal.js | 110 ++++++++++++++++++ 1 file changed, 110 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js index bcb9f7189c..43e77da4ac 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js @@ -655,6 +655,116 @@ describe('ReactIncrementalUpdates', () => { }); }); + it('does not throw out partially completed tree if it expires midway through', () => { + function Text({text}) { + Scheduler.unstable_yieldValue(text); + return text; + } + + function App({step}) { + return ( + <> + + + + + ); + } + + function interrupt() { + ReactNoop.flushSync(() => { + ReactNoop.renderToRootWithID(null, 'other-root'); + }); + } + + // First, as a sanity check, assert what happens when four low pri + // updates in separate batches are all flushed in the same callback + ReactNoop.act(() => { + ReactNoop.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['A1']); + + interrupt(); + + ReactNoop.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['A1']); + + interrupt(); + + ReactNoop.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['A1']); + + interrupt(); + + ReactNoop.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['A1']); + + // Each update flushes in a separate commit. + // Note: This isn't necessarily the ideal behavior. It might be better to + // batch all of these updates together. The fact that they don't is an + // implementation detail. The important part of this unit test is what + // happens when they expire, in which case they really should be batched to + // avoid blocking the main thread for a long time. + expect(Scheduler).toFlushAndYield([ + // A1 already completed. Finish rendering the first level. + 'B1', + 'C1', + // The remaining two levels complete sequentially. + 'A2', + 'B2', + 'C2', + 'A3', + 'B3', + 'C3', + 'A4', + 'B4', + 'C4', + ]); + }); + + ReactNoop.act(() => { + // Now do the same thing over again, but this time, expire all the updates + // instead of flushing them normally. + ReactNoop.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['A1']); + + interrupt(); + + ReactNoop.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['A1']); + + interrupt(); + + ReactNoop.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['A1']); + + interrupt(); + + ReactNoop.render(); + Scheduler.unstable_advanceTime(1000); + expect(Scheduler).toFlushAndYieldThrough(['A1']); + + // Expire all the updates + ReactNoop.expire(10000); + + expect(Scheduler).toHaveYielded([ + // A1 already completed. Finish rendering the first level. + 'B1', + 'C1', + // Then render the remaining two levels in a single batch + 'A4', + 'B4', + 'C4', + ]); + }); + }); + it('when rebasing, does not exclude updates that were already committed, regardless of priority', async () => { const {useState, useLayoutEffect} = React;