Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix serial passive effects #15650

Merged
merged 2 commits into from May 15, 2019
Merged
Changes from all commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -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;

@@ -52,7 +52,6 @@ import {
requestCurrentTime,
computeExpirationForFiber,
scheduleWork,
flushPassiveEffects,
} from './ReactFiberScheduler';

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

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

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

flushPassiveEffects();
enqueueUpdate(fiber, update);
scheduleWork(fiber, expirationTime);
},
@@ -31,7 +31,6 @@ import {
import {
scheduleWork,
computeExpirationForFiber,
flushPassiveEffects,
requestCurrentTime,
warnIfNotCurrentlyActingUpdatesInDev,
markRenderEventTime,
@@ -1108,8 +1107,6 @@ function dispatchAction<S, A>(
lastRenderPhaseUpdate.next = update;
}
} else {
flushPassiveEffects();

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

@@ -152,7 +152,6 @@ function scheduleRootUpdate(
update.callback = callback;
}

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

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

const newState = copyWithSet(currentHook.memoizedState, path, value);
currentHook.memoizedState = newState;
currentHook.baseState = newState;
@@ -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;
@@ -420,7 +416,6 @@ if (__DEV__) {
};

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

@@ -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) {
@@ -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));
}

@@ -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;
@@ -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.)
@@ -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();
@@ -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']);
@@ -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);
@@ -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({
@@ -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')]);
@@ -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',
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.