Bugfix: Effects should never have higher than normal priority (#16257)
* Bugfix: Priority when effects are flushed early The priority of passive effects is supposed to be the same as the priority of the render. This fixes a bug where the priority is sometimes wrong if the effects are flushed early. But the priority should really never be higher than Normal Priority. I'll change that in the next commit. * Effects never have higher than normal priority Effects currently have the same priority as the render that spawns them. This changes the behavior so that effects always have normal priority, or lower if the render priority is lower (e.g. offscreen prerendering). The implementation is a bit awkward because of the way `renderRoot`, `commitRoot`, and `flushPassiveEffects` are split. This is a known factoring problem that I'm planning to address once 16.9 is released.
Andrew Clark committed
Jul 30, 2019 at 15:46 UTC
f440bfd55911588d90c1d6e00d5a6b8feeda9b48
3 files changed
+79
-23
packages/react-reconciler/src/ReactFiberWorkLoop.js
+20
-4
@@ -40,6 +40,7 @@ import {
40
shouldYield,
41
requestPaint,
42
now,
43
+ NoPriority,
44
ImmediatePriority,
45
UserBlockingPriority,
46
NormalPriority,
@@ -237,6 +238,7 @@ let legacyErrorBoundariesThatAlreadyFailed: Set<mixed> | null = null;
238
239
let rootDoesHavePassiveEffects: boolean = false;
240
let rootWithPendingPassiveEffects: FiberRoot | null = null;
241
+let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority;
242
let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork;
243
244
let rootsWithPendingDiscreteUpdates: Map<
@@ -1494,12 +1496,15 @@ function resetChildExpirationTime(completedWork: Fiber) {
1496
}
1497
1498
function commitRoot(root) {
1497
- runWithPriority(ImmediatePriority, commitRootImpl.bind(null, root));
1499
+ const renderPriorityLevel = getCurrentPriorityLevel();
1500
+ runWithPriority(
1501
+ ImmediatePriority,
1502
+ commitRootImpl.bind(null, root, renderPriorityLevel),
1503
+ );
1504
// If there are passive effects, schedule a callback to flush them. This goes
1505
// outside commitRootImpl so that it inherits the priority of the render.
1506
if (rootWithPendingPassiveEffects !== null) {
1501
- const priorityLevel = getCurrentPriorityLevel();
1502
- scheduleCallback(priorityLevel, () => {
1507
+ scheduleCallback(NormalPriority, () => {
1508
flushPassiveEffects();
1509
return null;
1510
});
@@ -1507,7 +1512,7 @@ function commitRoot(root) {
1512
return null;
1513
}
1514
1510
-function commitRootImpl(root) {
1515
+function commitRootImpl(root, renderPriorityLevel) {
1516
flushPassiveEffects();
1517
flushRenderPhaseStrictModeWarningsInDEV();
1518
@@ -1730,6 +1735,7 @@ function commitRootImpl(root) {
1735
rootDoesHavePassiveEffects = false;
1736
rootWithPendingPassiveEffects = root;
1737
pendingPassiveEffectsExpirationTime = expirationTime;
1738
+ pendingPassiveEffectsRenderPriority = renderPriorityLevel;
1739
} else {
1740
// We are done with the effect chain at this point so let's clear the
1741
// nextEffect pointers to assist with GC. If we have passive effects, we'll
@@ -1937,9 +1943,19 @@ export function flushPassiveEffects() {
1943
}
1944
const root = rootWithPendingPassiveEffects;
1945
const expirationTime = pendingPassiveEffectsExpirationTime;
1946
+ const renderPriorityLevel = pendingPassiveEffectsRenderPriority;
1947
rootWithPendingPassiveEffects = null;
1948
pendingPassiveEffectsExpirationTime = NoWork;
1949
+ pendingPassiveEffectsRenderPriority = NoPriority;
1950
+ const priorityLevel =
1951
+ renderPriorityLevel > NormalPriority ? NormalPriority : renderPriorityLevel;
1952
+ return runWithPriority(
1953
+ priorityLevel,
1954
+ flushPassiveEffectsImpl.bind(null, root, expirationTime),
1955
+ );
1956
+}
1957
1958
+function flushPassiveEffectsImpl(root, expirationTime) {
1959
let prevInteractions: Set<Interaction> | null = null;
1960
if (enableSchedulerTracing) {
1961
prevInteractions = __interactionsRef.current;
packages/react-reconciler/src/SchedulerWithReactIntegration.js
+1
-1
@@ -43,7 +43,7 @@ if (enableSchedulerTracing) {
43
);
44
}
45
46
-export opaque type ReactPriorityLevel = 99 | 98 | 97 | 96 | 95 | 90;
46
+export type ReactPriorityLevel = 99 | 98 | 97 | 96 | 95 | 90;
47
export type SchedulerCallback = (isSync: boolean) => SchedulerCallback | null;
48
49
type SchedulerCallbackOptions = {
packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.internal.js
+58
-18
@@ -139,35 +139,78 @@ describe('ReactSchedulerIntegration', () => {
139
]);
140
});
141
142
- it('passive effects have the same priority as render', () => {
142
+ it('passive effects never have higher than normal priority', async () => {
143
const {useEffect} = React;
144
- function ReadPriority() {
144
+ function ReadPriority({step}) {
145
Scheduler.unstable_yieldValue(
146
- 'Render priority: ' + getCurrentPriorityAsString(),
146
+ `Render priority: ${getCurrentPriorityAsString()}`,
147
);
148
useEffect(() => {
149
Scheduler.unstable_yieldValue(
150
- 'Passive priority: ' + getCurrentPriorityAsString(),
150
+ `Effect priority: ${getCurrentPriorityAsString()}`,
151
);
152
});
153
return null;
154
}
155
- ReactNoop.act(() => {
156
- ReactNoop.render(<ReadPriority />);
157
- expect(Scheduler).toFlushAndYield([
158
- 'Render priority: Normal',
159
- 'Passive priority: Normal',
160
- ]);
155
162
- runWithPriority(UserBlockingPriority, () => {
156
+ // High priority renders spawn effects at normal priority
157
+ await ReactNoop.act(async () => {
158
+ Scheduler.unstable_runWithPriority(ImmediatePriority, () => {
159
+ ReactNoop.render(<ReadPriority />);
160
+ });
161
+ });
162
+ expect(Scheduler).toHaveYielded([
163
+ 'Render priority: Immediate',
164
+ 'Effect priority: Normal',
165
+ ]);
166
+ await ReactNoop.act(async () => {
167
+ Scheduler.unstable_runWithPriority(UserBlockingPriority, () => {
168
ReactNoop.render(<ReadPriority />);
169
});
170
+ });
171
+ expect(Scheduler).toHaveYielded([
172
+ 'Render priority: UserBlocking',
173
+ 'Effect priority: Normal',
174
+ ]);
175
166
- expect(Scheduler).toFlushAndYield([
167
- 'Render priority: UserBlocking',
168
- 'Passive priority: UserBlocking',
169
- ]);
176
+ // Renders lower than normal priority spawn effects at the same priority
177
+ await ReactNoop.act(async () => {
178
+ Scheduler.unstable_runWithPriority(IdlePriority, () => {
179
+ ReactNoop.render(<ReadPriority />);
180
+ });
181
});
182
+ expect(Scheduler).toHaveYielded([
183
+ 'Render priority: Idle',
184
+ 'Effect priority: Idle',
185
+ ]);
186
+ });
187
+
188
+ it('passive effects have correct priority even if they are flushed early', async () => {
189
+ const {useEffect} = React;
190
+ function ReadPriority({step}) {
191
+ Scheduler.unstable_yieldValue(
192
+ `Render priority [step ${step}]: ${getCurrentPriorityAsString()}`,
193
+ );
194
+ useEffect(() => {
195
+ Scheduler.unstable_yieldValue(
196
+ `Effect priority [step ${step}]: ${getCurrentPriorityAsString()}`,
197
+ );
198
+ });
199
+ return null;
200
+ }
201
+ await ReactNoop.act(async () => {
202
+ ReactNoop.render(<ReadPriority step={1} />);
203
+ Scheduler.unstable_flushUntilNextPaint();
204
+ expect(Scheduler).toHaveYielded(['Render priority [step 1]: Normal']);
205
+ Scheduler.unstable_runWithPriority(UserBlockingPriority, () => {
206
+ ReactNoop.render(<ReadPriority step={2} />);
207
+ });
208
+ });
209
+ expect(Scheduler).toHaveYielded([
210
+ 'Effect priority [step 1]: Normal',
211
+ 'Render priority [step 2]: UserBlocking',
212
+ 'Effect priority [step 2]: Normal',
213
+ ]);
214
});
215
216
it('after completing a level of work, infers priority of the next batch based on its expiration time', () => {
@@ -213,7 +256,4 @@ describe('ReactSchedulerIntegration', () => {
256
Scheduler.unstable_flushUntilNextPaint();
257
expect(Scheduler).toHaveYielded(['A', 'B', 'C']);
258
});
216
-
217
- // TODO
218
- it.skip('passive effects have render priority even if they are flushed early', () => {});
259
});