From f09fefd95fd0cd5800037a20b0b8197af20cf161 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 31 Mar 2021 12:39:19 -0500 Subject: [PATCH] Flush discrete passive effects before paint (#21150) If a discrete render results in passive effects, we should flush them synchronously at the end of the current task so that the result is immediately observable. For example, if a passive effect adds an event listener, the listener will be added before the next input. We don't need to do this for effects that don't have discrete/sync priority, because we assume they are not order-dependent and do not need to be observed by external systems. For legacy mode, we will maintain the existing behavior, since it hasn't been reported as an issue, and we'd have to do additional work to distinguish "legacy default sync" from "discrete sync" to prevent all passive effects from being treated this way. --- .../src/ReactFiberWorkLoop.new.js | 15 ++++ .../src/ReactFiberWorkLoop.old.js | 15 ++++ .../src/__tests__/ReactFlushSync-test.js | 69 +++++++++++++++++++ .../ReactHooksWithNoopRenderer-test.js | 7 +- .../__tests__/ReactProfiler-test.internal.js | 11 ++- 5 files changed, 108 insertions(+), 9 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 6b7b35c135986..16328690c3e55 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2010,6 +2010,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. flushSyncCallbackQueue(); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index d2a2720702fd0..dc2f018cad045 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -2010,6 +2010,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. flushSyncCallbackQueue(); diff --git a/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js b/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js index bc7499a3d43a7..6326aaa2aeab6 100644 --- a/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js +++ b/packages/react-reconciler/src/__tests__/ReactFlushSync-test.js @@ -97,4 +97,73 @@ describe('ReactFlushSync', () => { expect(Scheduler).toHaveYielded(['1, 1']); 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 cf4e4a3f3b619..973a0cbea81d7 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -33,6 +33,7 @@ let useDeferredValue; let forwardRef; let memo; let act; +let ContinuousEventPriority; describe('ReactHooksWithNoopRenderer', () => { beforeEach(() => { @@ -57,6 +58,8 @@ describe('ReactHooksWithNoopRenderer', () => { useDeferredValue = React.unstable_useDeferredValue; Suspense = React.Suspense; act = ReactNoop.act; + ContinuousEventPriority = require('react-reconciler/constants') + .ContinuousEventPriority; textCache = new Map(); @@ -1351,10 +1354,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', ]); diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js index ff2897141b57f..ab7fbb73f025a 100644 --- a/packages/react/src/__tests__/ReactProfiler-test.internal.js +++ b/packages/react/src/__tests__/ReactProfiler-test.internal.js @@ -3764,13 +3764,10 @@ describe('Profiler', () => { ); }); - // Profiler tag causes passive effects to be scheduled, - // so the interactions are still not completed. - expect(Scheduler).toHaveYielded(['SecondComponent']); - expect(onInteractionTraced).toHaveBeenCalledTimes(2); - expect(onInteractionScheduledWorkCompleted).not.toHaveBeenCalled(); - expect(Scheduler).toFlushAndYieldThrough(['onPostCommit']); - + expect(Scheduler).toHaveYielded([ + 'SecondComponent', + 'onPostCommit', + ]); expect(onInteractionTraced).toHaveBeenCalledTimes(2); expect(onInteractionScheduledWorkCompleted).toHaveBeenCalledTimes( 1,