Skip to content

Commit f440bfd

Browse files
acdliteacdlite
authored
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.
1 parent db3ae32 commit f440bfd

File tree

3 files changed

+79
-23
lines changed

3 files changed

+79
-23
lines changed

‎packages/react-reconciler/src/ReactFiberWorkLoop.js

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ import {
4040
shouldYield,
4141
requestPaint,
4242
now,
43+
NoPriority,
4344
ImmediatePriority,
4445
UserBlockingPriority,
4546
NormalPriority,
@@ -237,6 +238,7 @@ let legacyErrorBoundariesThatAlreadyFailed: Set<mixed> | null = null;
237238

238239
let rootDoesHavePassiveEffects: boolean = false;
239240
let rootWithPendingPassiveEffects: FiberRoot | null = null;
241+
let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority;
240242
let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork;
241243

242244
let rootsWithPendingDiscreteUpdates: Map<
@@ -1494,20 +1496,23 @@ function resetChildExpirationTime(completedWork: Fiber) {
14941496
}
14951497

14961498
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+
);
14981504
// If there are passive effects, schedule a callback to flush them. This goes
14991505
// outside commitRootImpl so that it inherits the priority of the render.
15001506
if (rootWithPendingPassiveEffects !== null) {
1501-
const priorityLevel = getCurrentPriorityLevel();
1502-
scheduleCallback(priorityLevel, () => {
1507+
scheduleCallback(NormalPriority, () => {
15031508
flushPassiveEffects();
15041509
return null;
15051510
});
15061511
}
15071512
return null;
15081513
}
15091514

1510-
function commitRootImpl(root) {
1515+
function commitRootImpl(root, renderPriorityLevel) {
15111516
flushPassiveEffects();
15121517
flushRenderPhaseStrictModeWarningsInDEV();
15131518

@@ -1730,6 +1735,7 @@ function commitRootImpl(root) {
17301735
rootDoesHavePassiveEffects = false;
17311736
rootWithPendingPassiveEffects = root;
17321737
pendingPassiveEffectsExpirationTime = expirationTime;
1738+
pendingPassiveEffectsRenderPriority = renderPriorityLevel;
17331739
} else {
17341740
// We are done with the effect chain at this point so let's clear the
17351741
// nextEffect pointers to assist with GC. If we have passive effects, we'll
@@ -1937,9 +1943,19 @@ export function flushPassiveEffects() {
19371943
}
19381944
const root = rootWithPendingPassiveEffects;
19391945
const expirationTime = pendingPassiveEffectsExpirationTime;
1946+
const renderPriorityLevel = pendingPassiveEffectsRenderPriority;
19401947
rootWithPendingPassiveEffects = null;
19411948
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+
}
19421957

1958+
function flushPassiveEffectsImpl(root, expirationTime) {
19431959
let prevInteractions: Set<Interaction> | null = null;
19441960
if (enableSchedulerTracing) {
19451961
prevInteractions = __interactionsRef.current;

‎packages/react-reconciler/src/SchedulerWithReactIntegration.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@ if (enableSchedulerTracing) {
4343
);
4444
}
4545

46-
export opaque type ReactPriorityLevel = 99 | 98 | 97 | 96 | 95 | 90;
46+
export type ReactPriorityLevel = 99 | 98 | 97 | 96 | 95 | 90;
4747
export type SchedulerCallback = (isSync: boolean) => SchedulerCallback | null;
4848

4949
type SchedulerCallbackOptions = {

‎packages/react-reconciler/src/__tests__/ReactSchedulerIntegration-test.internal.js

Lines changed: 58 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -139,35 +139,78 @@ describe('ReactSchedulerIntegration', () => {
139139
]);
140140
});
141141

142-
it('passive effects have the same priority as render', () => {
142+
it('passive effects never have higher than normal priority', async () => {
143143
const {useEffect} = React;
144-
function ReadPriority() {
144+
function ReadPriority({step}) {
145145
Scheduler.unstable_yieldValue(
146-
'Render priority: ' + getCurrentPriorityAsString(),
146+
`Render priority: ${getCurrentPriorityAsString()}`,
147147
);
148148
useEffect(() => {
149149
Scheduler.unstable_yieldValue(
150-
'Passive priority: ' + getCurrentPriorityAsString(),
150+
`Effect priority: ${getCurrentPriorityAsString()}`,
151151
);
152152
});
153153
return null;
154154
}
155-
ReactNoop.act(() => {
156-
ReactNoop.render(<ReadPriority />);
157-
expect(Scheduler).toFlushAndYield([
158-
'Render priority: Normal',
159-
'Passive priority: Normal',
160-
]);
161155

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, () => {
163168
ReactNoop.render(<ReadPriority />);
164169
});
170+
});
171+
expect(Scheduler).toHaveYielded([
172+
'Render priority: UserBlocking',
173+
'Effect priority: Normal',
174+
]);
165175

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+
});
170181
});
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+
]);
171214
});
172215

173216
it('after completing a level of work, infers priority of the next batch based on its expiration time', () => {
@@ -213,7 +256,4 @@ describe('ReactSchedulerIntegration', () => {
213256
Scheduler.unstable_flushUntilNextPaint();
214257
expect(Scheduler).toHaveYielded(['A', 'B', 'C']);
215258
});
216-
217-
// TODO
218-
it.skip('passive effects have render priority even if they are flushed early', () => {});
219259
});

0 commit comments

Comments
 (0)