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.
Andrew Clark committed
Apr 8, 2021 at 20:45 UTC
b943aeba88237265523ec8cdaf40d24a171681fc
5 files changed
+101
-10
packages/react-reconciler/src/ReactEventPriorities.new.js
+7
@@ -53,6 +53,13 @@ export function higherEventPriority(
53
return a !== 0 && a < b ? a : b;
54
}
55
56
+export function lowerEventPriority(
57
+ a: EventPriority,
58
+ b: EventPriority,
59
+): EventPriority {
60
+ return a === 0 || a > b ? a : b;
61
+}
62
+
63
export function isHigherEventPriority(
64
a: EventPriority,
65
b: EventPriority,
packages/react-reconciler/src/ReactEventPriorities.old.js
+7
@@ -53,6 +53,13 @@ export function higherEventPriority(
53
return a !== 0 && a < b ? a : b;
54
}
55
56
+export function lowerEventPriority(
57
+ a: EventPriority,
58
+ b: EventPriority,
59
+): EventPriority {
60
+ return a === 0 || a > b ? a : b;
61
+}
62
+
63
export function isHigherEventPriority(
64
a: EventPriority,
65
b: EventPriority,
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+3
-5
@@ -167,7 +167,7 @@ import {
167
IdleEventPriority,
168
getCurrentUpdatePriority,
169
setCurrentUpdatePriority,
170
- higherEventPriority,
170
+ lowerEventPriority,
171
lanesToEventPriority,
172
} from './ReactEventPriorities.new';
173
import {requestCurrentTransition, NoTransition} from './ReactFiberTransition';
@@ -2049,10 +2049,8 @@ function commitRootImpl(root, renderPriorityLevel) {
2049
export function flushPassiveEffects(): boolean {
2050
// Returns whether passive effects were flushed.
2051
if (pendingPassiveEffectsLanes !== NoLanes) {
2052
- const priority = higherEventPriority(
2053
- DefaultEventPriority,
2054
- lanesToEventPriority(pendingPassiveEffectsLanes),
2055
- );
2052
+ const renderPriority = lanesToEventPriority(pendingPassiveEffectsLanes);
2053
+ const priority = lowerEventPriority(DefaultEventPriority, renderPriority);
2054
const prevTransition = ReactCurrentBatchConfig.transition;
2055
const previousPriority = getCurrentUpdatePriority();
2056
try {
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+3
-5
@@ -167,7 +167,7 @@ import {
167
IdleEventPriority,
168
getCurrentUpdatePriority,
169
setCurrentUpdatePriority,
170
- higherEventPriority,
170
+ lowerEventPriority,
171
lanesToEventPriority,
172
} from './ReactEventPriorities.old';
173
import {requestCurrentTransition, NoTransition} from './ReactFiberTransition';
@@ -2049,10 +2049,8 @@ function commitRootImpl(root, renderPriorityLevel) {
2049
export function flushPassiveEffects(): boolean {
2050
// Returns whether passive effects were flushed.
2051
if (pendingPassiveEffectsLanes !== NoLanes) {
2052
- const priority = higherEventPriority(
2053
- DefaultEventPriority,
2054
- lanesToEventPriority(pendingPassiveEffectsLanes),
2055
- );
2052
+ const renderPriority = lanesToEventPriority(pendingPassiveEffectsLanes);
2053
+ const priority = lowerEventPriority(DefaultEventPriority, renderPriority);
2054
const prevTransition = ReactCurrentBatchConfig.transition;
2055
const previousPriority = getCurrentUpdatePriority();
2056
try {
packages/react-reconciler/src/__tests__/ReactUpdatePriority-test.js
new
+81
@@ -0,0 +1,81 @@
1
+let React;
2
+let ReactNoop;
3
+let Scheduler;
4
+let useState;
5
+let useEffect;
6
+
7
+describe('ReactUpdatePriority', () => {
8
+ beforeEach(() => {
9
+ jest.resetModules();
10
+
11
+ React = require('react');
12
+ ReactNoop = require('react-noop-renderer');
13
+ Scheduler = require('scheduler');
14
+ useState = React.useState;
15
+ useEffect = React.useEffect;
16
+ });
17
+
18
+ function Text({text}) {
19
+ Scheduler.unstable_yieldValue(text);
20
+ return text;
21
+ }
22
+
23
+ test('setState inside passive effect triggered by sync update should have default priority', async () => {
24
+ const root = ReactNoop.createRoot();
25
+
26
+ function App() {
27
+ const [state, setState] = useState(1);
28
+ useEffect(() => {
29
+ setState(2);
30
+ }, []);
31
+ return <Text text={state} />;
32
+ }
33
+
34
+ await ReactNoop.act(async () => {
35
+ ReactNoop.flushSync(() => {
36
+ root.render(<App />);
37
+ });
38
+ // Should not have flushed the effect update yet
39
+ expect(Scheduler).toHaveYielded([1]);
40
+ });
41
+ expect(Scheduler).toHaveYielded([2]);
42
+ });
43
+
44
+ test('setState inside passive effect triggered by idle update should have idle priority', async () => {
45
+ const root = ReactNoop.createRoot();
46
+
47
+ let setDefaultState;
48
+ function App() {
49
+ const [idleState, setIdleState] = useState(1);
50
+ const [defaultState, _setDetaultState] = useState(1);
51
+ setDefaultState = _setDetaultState;
52
+ useEffect(() => {
53
+ Scheduler.unstable_yieldValue('Idle update');
54
+ setIdleState(2);
55
+ }, []);
56
+ return <Text text={`Idle: ${idleState}, Default: ${defaultState}`} />;
57
+ }
58
+
59
+ await ReactNoop.act(async () => {
60
+ ReactNoop.idleUpdates(() => {
61
+ root.render(<App />);
62
+ });
63
+ // Should not have flushed the effect update yet
64
+ expect(Scheduler).toFlushUntilNextPaint(['Idle: 1, Default: 1']);
65
+
66
+ // Schedule another update at default priority
67
+ setDefaultState(2);
68
+
69
+ // The default update flushes first, because
70
+ expect(Scheduler).toFlushUntilNextPaint([
71
+ // Idle update is scheduled
72
+ 'Idle update',
73
+
74
+ // The default update flushes first, without including the idle update
75
+ 'Idle: 1, Default: 2',
76
+ ]);
77
+ });
78
+ // Now the idle update has flushed
79
+ expect(Scheduler).toHaveYielded(['Idle: 2, Default: 2']);
80
+ });
81
+});