From b943aeba88237265523ec8cdaf40d24a171681fc Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 8 Apr 2021 20:45:35 -0500 Subject: [PATCH] Fix: Passive effect updates are never sync (#21215) I screwed this up in #21082. Got confused by the < versus > thing again. The helper functions are annoying, too, because I always forget the intended order of the arguments. But they're still helpful because when we refactor the type we only have the change the logic in one place. Added a regression test. --- .../src/ReactEventPriorities.new.js | 7 ++ .../src/ReactEventPriorities.old.js | 7 ++ .../src/ReactFiberWorkLoop.new.js | 8 +- .../src/ReactFiberWorkLoop.old.js | 8 +- .../src/__tests__/ReactUpdatePriority-test.js | 81 +++++++++++++++++++ 5 files changed, 101 insertions(+), 10 deletions(-) create mode 100644 packages/react-reconciler/src/__tests__/ReactUpdatePriority-test.js diff --git a/packages/react-reconciler/src/ReactEventPriorities.new.js b/packages/react-reconciler/src/ReactEventPriorities.new.js index 2c1b9c249fa93..96225c19b3f11 100644 --- a/packages/react-reconciler/src/ReactEventPriorities.new.js +++ b/packages/react-reconciler/src/ReactEventPriorities.new.js @@ -53,6 +53,13 @@ export function higherEventPriority( return a !== 0 && a < b ? a : b; } +export function lowerEventPriority( + a: EventPriority, + b: EventPriority, +): EventPriority { + return a === 0 || a > b ? a : b; +} + export function isHigherEventPriority( a: EventPriority, b: EventPriority, diff --git a/packages/react-reconciler/src/ReactEventPriorities.old.js b/packages/react-reconciler/src/ReactEventPriorities.old.js index 7e1b0acb7a9a6..8db74b9397a83 100644 --- a/packages/react-reconciler/src/ReactEventPriorities.old.js +++ b/packages/react-reconciler/src/ReactEventPriorities.old.js @@ -53,6 +53,13 @@ export function higherEventPriority( return a !== 0 && a < b ? a : b; } +export function lowerEventPriority( + a: EventPriority, + b: EventPriority, +): EventPriority { + return a === 0 || a > b ? a : b; +} + export function isHigherEventPriority( a: EventPriority, b: EventPriority, diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 455067e47e263..425c46851aaba 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -167,7 +167,7 @@ import { IdleEventPriority, getCurrentUpdatePriority, setCurrentUpdatePriority, - higherEventPriority, + lowerEventPriority, lanesToEventPriority, } from './ReactEventPriorities.new'; import {requestCurrentTransition, NoTransition} from './ReactFiberTransition'; @@ -2049,10 +2049,8 @@ function commitRootImpl(root, renderPriorityLevel) { export function flushPassiveEffects(): boolean { // Returns whether passive effects were flushed. if (pendingPassiveEffectsLanes !== NoLanes) { - const priority = higherEventPriority( - DefaultEventPriority, - lanesToEventPriority(pendingPassiveEffectsLanes), - ); + const renderPriority = lanesToEventPriority(pendingPassiveEffectsLanes); + const priority = lowerEventPriority(DefaultEventPriority, renderPriority); const prevTransition = ReactCurrentBatchConfig.transition; const previousPriority = getCurrentUpdatePriority(); try { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 9d056a46402c3..b22901ff84ec3 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -167,7 +167,7 @@ import { IdleEventPriority, getCurrentUpdatePriority, setCurrentUpdatePriority, - higherEventPriority, + lowerEventPriority, lanesToEventPriority, } from './ReactEventPriorities.old'; import {requestCurrentTransition, NoTransition} from './ReactFiberTransition'; @@ -2049,10 +2049,8 @@ function commitRootImpl(root, renderPriorityLevel) { export function flushPassiveEffects(): boolean { // Returns whether passive effects were flushed. if (pendingPassiveEffectsLanes !== NoLanes) { - const priority = higherEventPriority( - DefaultEventPriority, - lanesToEventPriority(pendingPassiveEffectsLanes), - ); + const renderPriority = lanesToEventPriority(pendingPassiveEffectsLanes); + const priority = lowerEventPriority(DefaultEventPriority, renderPriority); const prevTransition = ReactCurrentBatchConfig.transition; const previousPriority = getCurrentUpdatePriority(); try { diff --git a/packages/react-reconciler/src/__tests__/ReactUpdatePriority-test.js b/packages/react-reconciler/src/__tests__/ReactUpdatePriority-test.js new file mode 100644 index 0000000000000..0aee7df66df4a --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ReactUpdatePriority-test.js @@ -0,0 +1,81 @@ +let React; +let ReactNoop; +let Scheduler; +let useState; +let useEffect; + +describe('ReactUpdatePriority', () => { + beforeEach(() => { + jest.resetModules(); + + React = require('react'); + ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); + useState = React.useState; + useEffect = React.useEffect; + }); + + function Text({text}) { + Scheduler.unstable_yieldValue(text); + return text; + } + + test('setState inside passive effect triggered by sync update should have default priority', async () => { + const root = ReactNoop.createRoot(); + + function App() { + const [state, setState] = useState(1); + useEffect(() => { + setState(2); + }, []); + return ; + } + + await ReactNoop.act(async () => { + ReactNoop.flushSync(() => { + root.render(); + }); + // Should not have flushed the effect update yet + expect(Scheduler).toHaveYielded([1]); + }); + expect(Scheduler).toHaveYielded([2]); + }); + + test('setState inside passive effect triggered by idle update should have idle priority', async () => { + const root = ReactNoop.createRoot(); + + let setDefaultState; + function App() { + const [idleState, setIdleState] = useState(1); + const [defaultState, _setDetaultState] = useState(1); + setDefaultState = _setDetaultState; + useEffect(() => { + Scheduler.unstable_yieldValue('Idle update'); + setIdleState(2); + }, []); + return ; + } + + await ReactNoop.act(async () => { + ReactNoop.idleUpdates(() => { + root.render(); + }); + // Should not have flushed the effect update yet + expect(Scheduler).toFlushUntilNextPaint(['Idle: 1, Default: 1']); + + // Schedule another update at default priority + setDefaultState(2); + + // The default update flushes first, because + expect(Scheduler).toFlushUntilNextPaint([ + // Idle update is scheduled + 'Idle update', + + // The default update flushes first, without including the idle update + 'Idle: 1, Default: 2', + ]); + }); + // Now the idle update has flushed + expect(Scheduler).toHaveYielded(['Idle: 2, Default: 2']); + }); +});