Skip to content

Commit

Permalink
Fix serial passive effects (#15650)
Browse files Browse the repository at this point in the history 
* Failing test for false positive warning

* Flush passive effects before discrete events

Currently, we check for pending passive effects inside the `setState`
method before we add additional updates to the queue, in case those
pending effects also add things to the queue.

However, the `setState` method is too late, because the event that
caused the update might not have ever fired had the passive effects
flushed before we got there.

This is the same as the discrete/serial events problem. When a serial
update comes in, and there's already a pending serial update, we have to
do it before we call the user-provided event handlers. Because the event
handlers themselves might change as a result of the pending update.

This commit moves the `flushPassiveEffects` call to before the discrete
event handlers are called, and removes it from the `setState` method.
Non-discrete events will not cause passive effects to flush, which is
fine, since by definition they are not order dependent.
  • Loading branch information
@acdlite
acdlite committed May 15, 2019
1 parent b0657fd commit 668fbd6
Show file tree
Hide file tree
Showing 7 changed files with 76 additions and 61 deletions.
36 changes: 0 additions & 36 deletions packages/react-dom/src/__tests__/ReactDOMHooks-test.js
View file
Expand Up @@ -72,42 +72,6 @@ describe('ReactDOMHooks', () => {
expect(container3.textContent).toBe('6');
});

it('can batch synchronous work inside effects with other work', () => {
let otherContainer = document.createElement('div');

let calledA = false;
function A() {
calledA = true;
return 'A';
}

let calledB = false;
function B() {
calledB = true;
return 'B';
}

let _set;
function Foo() {
_set = React.useState(0)[1];
React.useEffect(() => {
ReactDOM.render(<A />, otherContainer);
});
return null;
}

ReactDOM.render(<Foo />, container);
ReactDOM.unstable_batchedUpdates(() => {
_set(0); // Forces the effect to be flushed
expect(otherContainer.textContent).toBe('A');
ReactDOM.render(<B />, otherContainer);
expect(otherContainer.textContent).toBe('A');
});
expect(otherContainer.textContent).toBe('B');
expect(calledA).toBe(true);
expect(calledB).toBe(true);
});

it('should not bail out when an update is scheduled from within an event handler', () => {
const {createRef, useCallback, useState} = React;

Expand Down
4 changes: 0 additions & 4 deletions packages/react-reconciler/src/ReactFiberClassComponent.js
View file
Expand Up @@ -52,7 +52,6 @@ import {
requestCurrentTime,
computeExpirationForFiber,
scheduleWork,
flushPassiveEffects,
} from './ReactFiberScheduler';

const fakeInternalInstance = {};
Expand Down Expand Up @@ -194,7 +193,6 @@ const classComponentUpdater = {
update.callback = callback;
}

flushPassiveEffects();
enqueueUpdate(fiber, update);
scheduleWork(fiber, expirationTime);
},
Expand All @@ -214,7 +212,6 @@ const classComponentUpdater = {
update.callback = callback;
}

flushPassiveEffects();
enqueueUpdate(fiber, update);
scheduleWork(fiber, expirationTime);
},
Expand All @@ -233,7 +230,6 @@ const classComponentUpdater = {
update.callback = callback;
}

flushPassiveEffects();
enqueueUpdate(fiber, update);
scheduleWork(fiber, expirationTime);
},
Expand Down
3 changes: 0 additions & 3 deletions packages/react-reconciler/src/ReactFiberHooks.js
View file
Expand Up @@ -31,7 +31,6 @@ import {
import {
scheduleWork,
computeExpirationForFiber,
flushPassiveEffects,
requestCurrentTime,
warnIfNotCurrentlyActingUpdatesInDev,
markRenderEventTime,
Expand Down Expand Up @@ -1108,8 +1107,6 @@ function dispatchAction<S, A>(
lastRenderPhaseUpdate.next = update;
}
} else {
flushPassiveEffects();

const currentTime = requestCurrentTime();
const expirationTime = computeExpirationForFiber(currentTime, fiber);

Expand Down
5 changes: 0 additions & 5 deletions packages/react-reconciler/src/ReactFiberReconciler.js
View file
Expand Up @@ -152,7 +152,6 @@ function scheduleRootUpdate(
update.callback = callback;
}

flushPassiveEffects();
enqueueUpdate(current, update);
scheduleWork(current, expirationTime);

Expand Down Expand Up @@ -392,8 +391,6 @@ if (__DEV__) {
id--;
}
if (currentHook !== null) {
flushPassiveEffects();

const newState = copyWithSet(currentHook.memoizedState, path, value);
currentHook.memoizedState = newState;
currentHook.baseState = newState;
Expand All @@ -411,7 +408,6 @@ if (__DEV__) {

// Support DevTools props for function components, forwardRef, memo, host components, etc.
overrideProps = (fiber: Fiber, path: Array<string | number>, value: any) => {
flushPassiveEffects();
fiber.pendingProps = copyWithSet(fiber.memoizedProps, path, value);
if (fiber.alternate) {
fiber.alternate.pendingProps = fiber.pendingProps;
Expand All @@ -420,7 +416,6 @@ if (__DEV__) {
};

scheduleUpdate = (fiber: Fiber) => {
flushPassiveEffects();
scheduleWork(fiber, Sync);
};

Expand Down
5 changes: 5 additions & 0 deletions packages/react-reconciler/src/ReactFiberScheduler.js
View file
Expand Up @@ -560,6 +560,9 @@ export function flushInteractiveUpdates() {
return;
}
flushPendingDiscreteUpdates();
// If the discrete updates scheduled passive effects, flush them now so that
// they fire before the next serial event.
flushPassiveEffects();
}

function resolveLocksOnRoot(root: FiberRoot, expirationTime: ExpirationTime) {
Expand Down Expand Up @@ -595,6 +598,8 @@ export function interactiveUpdates<A, B, C, R>(
// should explicitly call flushInteractiveUpdates.
flushPendingDiscreteUpdates();
}
// TODO: Remove this call for the same reason as above.
flushPassiveEffects();
return runWithPriority(UserBlockingPriority, fn.bind(null, a, b, c));
}

Expand Down
52 changes: 52 additions & 0 deletions packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js
View file
Expand Up @@ -1719,6 +1719,58 @@ describe('ReactHooks', () => {
).toThrow('Hello');
});

// Regression test for https://github.com/facebook/react/issues/15057
it('does not fire a false positive warning when previous effect unmounts the component', () => {
let {useState, useEffect} = React;
let globalListener;

function A() {
const [show, setShow] = useState(true);
function hideMe() {
setShow(false);
}
return show ? <B hideMe={hideMe} /> : null;
}

function B(props) {
return <C {...props} />;
}

function C({hideMe}) {
const [, setState] = useState();

useEffect(() => {
let isStale = false;

globalListener = () => {
if (!isStale) {
setState('hello');
}
};

return () => {
isStale = true;
hideMe();
};
});
return null;
}

ReactTestRenderer.act(() => {
ReactTestRenderer.create(<A />);
});

expect(() => {
globalListener();
globalListener();
}).toWarnDev([
'An update to C inside a test was not wrapped in act',
'An update to C inside a test was not wrapped in act',
// Note: should *not* warn about updates on unmounted component.
// Because there's no way for component to know it got unmounted.
]);
});

// Regression test for https://github.com/facebook/react/issues/14790
it('does not fire a false positive warning when suspending memo', async () => {
const {Suspense, useState} = React;
Expand Down
32 changes: 19 additions & 13 deletions packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
View file
Expand Up @@ -670,8 +670,7 @@ describe('ReactHooksWithNoopRenderer', () => {
// Destroying the first child shouldn't prevent the passive effect from
// being executed
ReactNoop.render([passive]);
expect(Scheduler).toHaveYielded(['Passive effect']);
expect(Scheduler).toFlushAndYield([]);
expect(Scheduler).toFlushAndYield(['Passive effect']);
expect(ReactNoop.getChildren()).toEqual([span('Passive')]);

// (No effects are left to flush.)
Expand Down Expand Up @@ -776,11 +775,12 @@ describe('ReactHooksWithNoopRenderer', () => {
ReactNoop.render(<Counter count={1} />, () =>
Scheduler.yieldValue('Sync effect'),
);
expect(Scheduler).toHaveYielded([
expect(Scheduler).toFlushAndYieldThrough([
// The previous effect flushes before the reconciliation
'Committed state when effect was fired: 0',
1,
'Sync effect',
]);
expect(Scheduler).toFlushAndYieldThrough([1, 'Sync effect']);
expect(ReactNoop.getChildren()).toEqual([span(1)]);

ReactNoop.flushPassiveEffects();
Expand Down Expand Up @@ -849,8 +849,10 @@ describe('ReactHooksWithNoopRenderer', () => {
ReactNoop.render(<Counter count={1} />, () =>
Scheduler.yieldValue('Sync effect'),
);
expect(Scheduler).toHaveYielded(['Schedule update [0]']);
expect(Scheduler).toFlushAndYieldThrough(['Count: 0']);
expect(Scheduler).toFlushAndYieldThrough([
'Schedule update [0]',
'Count: 0',
]);
expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]);

expect(Scheduler).toFlushAndYieldThrough(['Sync effect']);
Expand All @@ -862,7 +864,7 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]);
});

it('flushes serial effects before enqueueing work', () => {
it('flushes passive effects when flushing discrete updates', () => {
let _updateCount;
function Counter(props) {
const [count, updateCount] = useState(0);
Expand All @@ -880,15 +882,17 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(Scheduler).toFlushAndYieldThrough(['Count: 0', 'Sync effect']);
expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]);

// Enqueuing this update forces the passive effect to be flushed --
// A discrete event forces the passive effect to be flushed --
// updateCount(1) happens first, so 2 wins.
act(() => _updateCount(2));
ReactNoop.interactiveUpdates(() => {
act(() => _updateCount(2));
});
expect(Scheduler).toHaveYielded(['Will set count to 1']);
expect(Scheduler).toFlushAndYield(['Count: 2']);
expect(ReactNoop.getChildren()).toEqual([span('Count: 2')]);
});

it('flushes serial effects before enqueueing work (with tracing)', () => {
it('flushes passive effects when flushing discrete updates (with tracing)', () => {
const onInteractionScheduledWorkCompleted = jest.fn();
const onWorkCanceled = jest.fn();
SchedulerTracing.unstable_subscribe({
Expand Down Expand Up @@ -929,9 +933,11 @@ describe('ReactHooksWithNoopRenderer', () => {

expect(onInteractionScheduledWorkCompleted).toHaveBeenCalledTimes(0);

// Enqueuing this update forces the passive effect to be flushed --
// A discrete event forces the passive effect to be flushed --
// updateCount(1) happens first, so 2 wins.
act(() => _updateCount(2));
ReactNoop.interactiveUpdates(() => {
act(() => _updateCount(2));
});
expect(Scheduler).toHaveYielded(['Will set count to 1']);
expect(Scheduler).toFlushAndYield(['Count: 2']);
expect(ReactNoop.getChildren()).toEqual([span('Count: 2')]);
Expand Down Expand Up @@ -1472,8 +1478,8 @@ describe('ReactHooksWithNoopRenderer', () => {
ReactNoop.render(<Counter count={1} />, () =>
Scheduler.yieldValue('Sync effect'),
);
expect(Scheduler).toHaveYielded(['Mount normal [current: 0]']);
expect(Scheduler).toFlushAndYieldThrough([
'Mount normal [current: 0]',
'Unmount layout [current: 0]',
'Mount layout [current: 1]',
'Sync effect',
Expand Down

0 comments on commit 668fbd6

Please sign in to comment.