From d3e381f1b22a89ca604fa0c5245fce9afb60539b Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 3 May 2021 13:34:22 -0500 Subject: [PATCH] Re-land "Flush discrete passive effects before paint (#21150)" This re-lands commit 2e7aceeb5c8b6e5c61174c0e9731e263e956e445. --- .../src/ReactFiberWorkLoop.new.js | 15 ++++ .../src/ReactFiberWorkLoop.old.js | 15 ++++ .../src/__tests__/ReactFlushSync-test.js | 69 +++++++++++++++++++ .../ReactHooksWithNoopRenderer-test.js | 7 +- 4 files changed, 104 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 7c659b7775a1e..99f3e014148f5 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -1976,6 +1976,21 @@ function commitRootImpl(root, renderPriorityLevel) { return null; } + // If the passive effects are the result of a discrete render, flush them + // synchronously at the end of the current task so that the result is + // immediately observable. Otherwise, we assume that they are not + // order-dependent and do not need to be observed by external systems, so we + // can wait until after paint. + // TODO: We can optimize this by not scheduling the callback earlier. Since we + // currently schedule the callback in multiple places, will wait until those + // are consolidated. + if ( + includesSomeLane(pendingPassiveEffectsLanes, SyncLane) && + root.tag !== LegacyRoot + ) { + flushPassiveEffects(); + } + // If layout work was scheduled, flush it now. flushSyncCallbacks(); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 530646f8a4d29..e7328b989083b 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -1976,6 +1976,21 @@ function commitRootImpl(root, renderPriorityLevel) { return null; } + // If the passive effects are the result of a discrete render, flush them + // synchronously at the end of the current task so that the result is + // immediately observable. Otherwise, we assume that they are not + // order-dependent and do not need to be observed by external systems, so we + // can wait until after paint. + // TODO: We can optimize this by not scheduling the callback earlier. Since we + // currently schedule the callback in multiple places, will wait until those + // are consolidated. + if ( + includesSomeLane(pendingPassiveEffectsLanes, SyncLane) && + root.tag !== LegacyRoot + ) { + flushPassiveEffects(); + } + // If layout work was scheduled, flush it now. flushSyncCallbacks(); diff --git a/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js b/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js index f763e44705aaa..327b4a87ca678 100644 --- a/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js +++ b/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js @@ -61,4 +61,73 @@ describe('ReactFlushSync', () => { }); expect(root).toMatchRenderedOutput('1, 1'); }); + + test('flushes passive effects synchronously when they are the result of a sync render', async () => { + function App() { + useEffect(() => { + Scheduler.unstable_yieldValue('Effect'); + }, []); + return ; + } + + const root = ReactNoop.createRoot(); + await ReactNoop.act(async () => { + ReactNoop.flushSync(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded([ + 'Child', + // Because the pending effect was the result of a sync update, calling + // flushSync should flush it. + 'Effect', + ]); + expect(root).toMatchRenderedOutput('Child'); + }); + }); + + test('do not flush passive effects synchronously in legacy mode', async () => { + function App() { + useEffect(() => { + Scheduler.unstable_yieldValue('Effect'); + }, []); + return ; + } + + const root = ReactNoop.createLegacyRoot(); + await ReactNoop.act(async () => { + ReactNoop.flushSync(() => { + root.render(); + }); + expect(Scheduler).toHaveYielded([ + 'Child', + // Because we're in legacy mode, we shouldn't have flushed the passive + // effects yet. + ]); + expect(root).toMatchRenderedOutput('Child'); + }); + // Effect flushes after paint. + expect(Scheduler).toHaveYielded(['Effect']); + }); + + test("do not flush passive effects synchronously when they aren't the result of a sync render", async () => { + function App() { + useEffect(() => { + Scheduler.unstable_yieldValue('Effect'); + }, []); + return ; + } + + const root = ReactNoop.createRoot(); + await ReactNoop.act(async () => { + root.render(); + expect(Scheduler).toFlushUntilNextPaint([ + 'Child', + // Because the passive effect was not the result of a sync update, it + // should not flush before paint. + ]); + expect(root).toMatchRenderedOutput('Child'); + }); + // Effect flushes after paint. + expect(Scheduler).toHaveYielded(['Effect']); + }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index a02c64067195c..27d09697fc213 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -32,6 +32,7 @@ let useDeferredValue; let forwardRef; let memo; let act; +let ContinuousEventPriority; describe('ReactHooksWithNoopRenderer', () => { beforeEach(() => { @@ -55,6 +56,8 @@ describe('ReactHooksWithNoopRenderer', () => { useDeferredValue = React.unstable_useDeferredValue; Suspense = React.Suspense; act = ReactNoop.act; + ContinuousEventPriority = require('react-reconciler/constants') + .ContinuousEventPriority; textCache = new Map(); @@ -1397,10 +1400,10 @@ describe('ReactHooksWithNoopRenderer', () => { expect(Scheduler).toFlushAndYieldThrough(['Child one render']); // Schedule unmount for the parent that unmounts children with pending update. - ReactNoop.flushSync(() => { + ReactNoop.unstable_runWithPriority(ContinuousEventPriority, () => { setParentState(false); }); - expect(Scheduler).toHaveYielded([ + expect(Scheduler).toFlushUntilNextPaint([ 'Parent false render', 'Parent false commit', ]);