Skip to content

Commit

Permalink
Fix: Passive effect updates are never sync (facebook#21215)
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 13, 2021
1 parent 03ede83 commit 4472d75
Show file tree
Hide file tree
Showing 5 changed files with 101 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
let React;
let ReactNoop;
let Scheduler;
let useState;
let useEffect;

describe('ReactUpdatePriority', () => {
beforeEach(() => {
jest.resetModules();

React = require('react');
ReactNoop = require('react-noop-renderer');
Scheduler = require('scheduler');
useState = React.useState;
useEffect = React.useEffect;
});

function Text({text}) {
Scheduler.unstable_yieldValue(text);
return text;
}

test('setState inside passive effect triggered by sync update should have default priority', 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]);
});

test('setState inside passive effect triggered by idle update should have idle priority', async () => {
const root = ReactNoop.createRoot();

let setDefaultState;
function App() {
const [idleState, setIdleState] = useState(1);
const [defaultState, _setDetaultState] = useState(1);
setDefaultState = _setDetaultState;
useEffect(() => {
Scheduler.unstable_yieldValue('Idle update');
setIdleState(2);
}, []);
return <Text text={`Idle: ${idleState}, Default: ${defaultState}`} />;
}

await ReactNoop.act(async () => {
ReactNoop.idleUpdates(() => {
root.render(<App />);
});
// Should not have flushed the effect update yet
expect(Scheduler).toFlushUntilNextPaint(['Idle: 1, Default: 1']);

// Schedule another update at default priority
setDefaultState(2);

// The default update flushes first, because
expect(Scheduler).toFlushUntilNextPaint([
// Idle update is scheduled
'Idle update',

// The default update flushes first, without including the idle update
'Idle: 1, Default: 2',
]);
});
// Now the idle update has flushed
expect(Scheduler).toHaveYielded(['Idle: 2, Default: 2']);
});
});

0 comments on commit 4472d75

Please sign in to comment.