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 d389c54 commit 37de0d4
Show file tree
Hide file tree
Showing 5 changed files with 41 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 @@ -167,7 +167,7 @@ import {
IdleEventPriority,
getCurrentUpdatePriority,
setCurrentUpdatePriority,
higherEventPriority,
lowerEventPriority,
lanesToEventPriority,
} from './ReactEventPriorities.new';
import {requestCurrentTransition, NoTransition} from './ReactFiberTransition';
Expand Down Expand Up @@ -2049,10 +2049,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 prevTransition = ReactCurrentBatchConfig.transition;
const previousPriority = getCurrentUpdatePriority();
try {
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 @@ -167,7 +167,7 @@ import {
IdleEventPriority,
getCurrentUpdatePriority,
setCurrentUpdatePriority,
higherEventPriority,
lowerEventPriority,
lanesToEventPriority,
} from './ReactEventPriorities.old';
import {requestCurrentTransition, NoTransition} from './ReactFiberTransition';
Expand Down Expand Up @@ -2049,10 +2049,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 prevTransition = ReactCurrentBatchConfig.transition;
const previousPriority = getCurrentUpdatePriority();
try {
Expand Down
21 changes: 21 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactFlushSync-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -166,4 +166,25 @@ describe('ReactFlushSync', () => {
// Effect flushes after paint.
expect(Scheduler).toHaveYielded(['Effect']);
});

test('setState inside passive effect triggered by sync update is not sync', async () => {
const root = ReactNoop.createRoot();

function App() {
const [state, setState] = useState(1);
useEffect(() => {
setState(2);
}, []);
return <Text text={state} />;
}

await ReactNoop.act(async () => {
ReactNoop.flushSync(() => {
root.render(<App />);
});
// Should not have flushed the effect update yet
expect(Scheduler).toHaveYielded([1]);
});
expect(Scheduler).toHaveYielded([2]);
});
});

0 comments on commit 37de0d4

Please sign in to comment.