From 3fac6a780b999fcd50007bf3c3e3c7e9314f290a Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 12 May 2021 12:26:06 -0500 Subject: [PATCH 1/2] 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. --- .../ReactClassSetStateCallback-test.js | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) create mode 100644 packages/react-reconciler/src/__tests__/ReactClassSetStateCallback-test.js diff --git a/packages/react-reconciler/src/__tests__/ReactClassSetStateCallback-test.js b/packages/react-reconciler/src/__tests__/ReactClassSetStateCallback-test.js new file mode 100644 index 0000000000000..13d9ce838c03b --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ReactClassSetStateCallback-test.js @@ -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 ; + } + } + + const root = ReactNoop.createRoot(); + await ReactNoop.act(async () => { + root.render(); + }); + 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']); + }); +}); From a807e67a599dbe42c3369e6d2304937153423ba8 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 12 May 2021 12:27:06 -0500 Subject: [PATCH 2/2] Prevent setState callback firing during rebase Before enqueueing the effect, adds a guard to check if the update was already committed. --- packages/react-reconciler/src/ReactUpdateQueue.new.js | 7 ++++++- packages/react-reconciler/src/ReactUpdateQueue.old.js | 7 ++++++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactUpdateQueue.new.js b/packages/react-reconciler/src/ReactUpdateQueue.new.js index 0bdcbf580764a..06ee95a92c705 100644 --- a/packages/react-reconciler/src/ReactUpdateQueue.new.js +++ b/packages/react-reconciler/src/ReactUpdateQueue.new.js @@ -580,7 +580,12 @@ export function processUpdateQueue( 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) { diff --git a/packages/react-reconciler/src/ReactUpdateQueue.old.js b/packages/react-reconciler/src/ReactUpdateQueue.old.js index 209abfb32e743..520f993fbd81f 100644 --- a/packages/react-reconciler/src/ReactUpdateQueue.old.js +++ b/packages/react-reconciler/src/ReactUpdateQueue.old.js @@ -580,7 +580,12 @@ export function processUpdateQueue( 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) {