Skip to content

Commit

Permalink
Fix: Passive effect updates are never sync
Browse files Browse the repository at this point in the history
I screwed this up in facebook#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.
  • Loading branch information
acdlite committed Apr 9, 2021
1 parent 03ede83 commit 951a292
Show file tree
Hide file tree
Showing 4 changed files with 20 additions and 10 deletions.
7 changes: 7 additions & 0 deletions packages/react-reconciler/src/ReactEventPriorities.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
7 changes: 7 additions & 0 deletions packages/react-reconciler/src/ReactEventPriorities.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
8 changes: 3 additions & 5 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ import {
DefaultEventPriority,
getCurrentUpdatePriority,
setCurrentUpdatePriority,
higherEventPriority,
lowerEventPriority,
lanesToEventPriority,
} from './ReactEventPriorities.new';
import {requestCurrentTransition, NoTransition} from './ReactFiberTransition';
Expand Down Expand Up @@ -2008,10 +2008,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 previousPriority = getCurrentUpdatePriority();
try {
setCurrentUpdatePriority(priority);
Expand Down
8 changes: 3 additions & 5 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ import {
DefaultEventPriority,
getCurrentUpdatePriority,
setCurrentUpdatePriority,
higherEventPriority,
lowerEventPriority,
lanesToEventPriority,
} from './ReactEventPriorities.old';
import {requestCurrentTransition, NoTransition} from './ReactFiberTransition';
Expand Down Expand Up @@ -2008,10 +2008,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 previousPriority = getCurrentUpdatePriority();
try {
setCurrentUpdatePriority(priority);
Expand Down

0 comments on commit 951a292

Please sign in to comment.