Skip to content

Commit

Permalink
[Bugfix] Prevent already-committed setState callback from firing agai…
Browse files Browse the repository at this point in the history
…n during a rebase (#21498)

* Failing test: Class callback fired multiple times

Happens during a rebase (low priority update followed by high priority
update). The high priority callback gets fired twice.

* Prevent setState callback firing during rebase

Before enqueueing the effect, adds a guard to check if the update was
already committed.
  • Loading branch information
acdlite committed May 12, 2021
1 parent b770f75 commit 46491dc
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 2 deletions.
7 changes: 6 additions & 1 deletion packages/react-reconciler/src/ReactUpdateQueue.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,12 @@ export function processUpdateQueue<State>(
instance,
);
const callback = update.callback;
if (callback !== null) {
if (
callback !== null &&
// If the update was already committed, we should not queue its
// callback again.
update.lane !== NoLane
) {
workInProgress.flags |= Callback;
const effects = queue.effects;
if (effects === null) {
Expand Down
7 changes: 6 additions & 1 deletion packages/react-reconciler/src/ReactUpdateQueue.old.js
Original file line number Diff line number Diff line change
Expand Up @@ -580,7 +580,12 @@ export function processUpdateQueue<State>(
instance,
);
const callback = update.callback;
if (callback !== null) {
if (
callback !== null &&
// If the update was already committed, we should not queue its
// callback again.
update.lane !== NoLane
) {
workInProgress.flags |= Callback;
const effects = queue.effects;
if (effects === null) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
let React;
let ReactNoop;
let Scheduler;

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

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

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

test('regression: setState callback (2nd arg) should only fire once, even after a rebase', async () => {
let app;
class App extends React.Component {
state = {step: 0};
render() {
app = this;
return <Text text={this.state.step} />;
}
}

const root = ReactNoop.createRoot();
await ReactNoop.act(async () => {
root.render(<App />);
});
expect(Scheduler).toHaveYielded([0]);

await ReactNoop.act(async () => {
app.setState({step: 1}, () =>
Scheduler.unstable_yieldValue('Callback 1'),
);
ReactNoop.flushSync(() => {
app.setState({step: 2}, () =>
Scheduler.unstable_yieldValue('Callback 2'),
);
});
});
expect(Scheduler).toHaveYielded([2, 'Callback 2', 2, 'Callback 1']);
});
});

0 comments on commit 46491dc

Please sign in to comment.