diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 9fd7b690f241c..cab2e21c0f2ef 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2015,6 +2015,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 aed7d0c16f039..a02be3af2ffa5 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -2015,6 +2015,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,