Skip to content

Commit

Permalink
Effects never have higher than normal priority
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
acdlite committed Jul 30, 2019
1 parent b0204b4 commit 52c6f7b
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 20 deletions.
7 changes: 5 additions & 2 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -1486,7 +1486,7 @@ function commitRoot(root) {
if (rootWithPendingPassiveEffects !== null) {
const renderPriorityLevel = getCurrentPriorityLevel();
pendingPassiveEffectsRenderPriority = renderPriorityLevel;
scheduleCallback(renderPriorityLevel, () => {
scheduleCallback(NormalPriority, () => {
flushPassiveEffects();
return null;
});
Expand Down Expand Up @@ -1924,9 +1924,12 @@ export function flushPassiveEffects() {
}
const root = rootWithPendingPassiveEffects;
const expirationTime = pendingPassiveEffectsExpirationTime;
const priorityLevel = pendingPassiveEffectsRenderPriority;
const renderPriorityLevel = pendingPassiveEffectsRenderPriority;
rootWithPendingPassiveEffects = null;
pendingPassiveEffectsExpirationTime = NoWork;
pendingPassiveEffectsRenderPriority = NoPriority;
const priorityLevel =
renderPriorityLevel < NormalPriority ? renderPriorityLevel : NormalPriority;
return runWithPriority(
priorityLevel,
flushPassiveEffectsImpl.bind(null, root, expirationTime),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ if (enableSchedulerTracing) {
);
}

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

type SchedulerCallbackOptions = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,38 +139,53 @@ describe('ReactSchedulerIntegration', () => {
]);
});

it('passive effects have the same priority as render', () => {
it('passive effects never have higher than normal priority', async () => {
const {useEffect} = React;
function ReadPriority() {
function ReadPriority({step}) {
Scheduler.unstable_yieldValue(
'Render priority: ' + getCurrentPriorityAsString(),
`Render priority: ${getCurrentPriorityAsString()}`,
);
useEffect(() => {
Scheduler.unstable_yieldValue(
'Passive priority: ' + getCurrentPriorityAsString(),
`Effect priority: ${getCurrentPriorityAsString()}`,
);
});
return null;
}
ReactNoop.act(() => {
ReactNoop.render(<ReadPriority />);
expect(Scheduler).toFlushAndYield([
'Render priority: Normal',
'Passive priority: Normal',
]);

runWithPriority(UserBlockingPriority, () => {
// High priority renders spawn effects at normal priority
await ReactNoop.act(async () => {
Scheduler.unstable_runWithPriority(ImmediatePriority, () => {
ReactNoop.render(<ReadPriority />);
});
});
expect(Scheduler).toHaveYielded([
'Render priority: Immediate',
'Effect priority: Normal',
]);
await ReactNoop.act(async () => {
Scheduler.unstable_runWithPriority(UserBlockingPriority, () => {
ReactNoop.render(<ReadPriority />);
});
});
expect(Scheduler).toHaveYielded([
'Render priority: UserBlocking',
'Effect priority: Normal',
]);

expect(Scheduler).toFlushAndYield([
'Render priority: UserBlocking',
'Passive priority: UserBlocking',
]);
// Renders lower than normal priority spawn effects at the same priority
await ReactNoop.act(async () => {
Scheduler.unstable_runWithPriority(IdlePriority, () => {
ReactNoop.render(<ReadPriority />);
});
});
expect(Scheduler).toHaveYielded([
'Render priority: Idle',
'Effect priority: Idle',
]);
});

it('passive effects have render priority even if they are flushed early', async () => {
it('passive effects have correct priority even if they are flushed early', async () => {
const {useEffect} = React;
function ReadPriority({step}) {
Scheduler.unstable_yieldValue(
Expand All @@ -194,7 +209,7 @@ describe('ReactSchedulerIntegration', () => {
expect(Scheduler).toHaveYielded([
'Effect priority [step 1]: Normal',
'Render priority [step 2]: UserBlocking',
'Effect priority [step 2]: UserBlocking',
'Effect priority [step 2]: Normal',
]);
});

Expand Down

0 comments on commit 52c6f7b

Please sign in to comment.