Skip to content

Commit

Permalink
flush pending passive effects early on an interactive update
Browse files Browse the repository at this point in the history
related to facebook#15057
  • Loading branch information
Sunil Pai committed May 14, 2019
1 parent edfedf3 commit 7770d56
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 1 deletion.
42 changes: 42 additions & 0 deletions packages/react-dom/src/__tests__/ReactDOMHooks-test.js
Expand Up @@ -11,6 +11,7 @@

let React;
let ReactDOM;
let act;
let Scheduler;

describe('ReactDOMHooks', () => {
Expand All @@ -22,6 +23,7 @@ describe('ReactDOMHooks', () => {
React = require('react');
ReactDOM = require('react-dom');
Scheduler = require('scheduler');
act = require('react-dom/test-utils').act;

container = document.createElement('div');
document.body.appendChild(container);
Expand Down Expand Up @@ -179,4 +181,44 @@ describe('ReactDOMHooks', () => {

expect(labelRef.current.innerHTML).toBe('abc');
});
it('should flush passive effects before interactive events', () => {
// related to #15057
// the test is a bit contrived since it'll never happen in a 'real' test/browser
// (the effect would flush on start and the clicks would never set state)
// but it does model setting state in an effect clean up that removes a previous handler

// users should probably wrap their code with unstable_interactiveUpdates? or
// use emitEffect?
const {useState, useEffect} = React;

function Foo() {
const [count, setCount] = useState(0);
const [enabled, setEnabled] = useState(true);
useEffect(() => {
return () => {
setEnabled(false);
};
});
function handleClick() {
setCount(x => x + 1);
}
return <button onClick={enabled ? handleClick : null}>{count}</button>;
}

ReactDOM.render(<Foo />, container);

container.firstChild.dispatchEvent(
new Event('click', {bubbles: true, cancelable: true}),
);
// Cleanup from first passive effect should remove the handler.
container.firstChild.dispatchEvent(
new Event('click', {bubbles: true, cancelable: true}),
);
container.firstChild.dispatchEvent(
new Event('click', {bubbles: true, cancelable: true}),
);

jest.runAllTimers();
expect(container.textContent).toBe('1');
});
});
3 changes: 2 additions & 1 deletion packages/react-reconciler/src/ReactFiberScheduler.js
Expand Up @@ -539,6 +539,7 @@ export function flushInteractiveUpdates() {
// an input event inside an effect, like with `element.click()`.
return;
}
flushPassiveEffects();
flushPendingDiscreteUpdates();
}

Expand Down Expand Up @@ -575,7 +576,7 @@ export function interactiveUpdates<A, B, C, R>(
if (workPhase === NotWorking) {
// TODO: Remove this call. Instead of doing this automatically, the caller
// should explicitly call flushInteractiveUpdates.
flushPendingDiscreteUpdates();
flushInteractiveUpdates();
}
return runWithPriority(UserBlockingPriority, fn.bind(null, a, b, c));
}
Expand Down

0 comments on commit 7770d56

Please sign in to comment.