Skip to content

Commit

Permalink
Eager bailout optimization should always compare to latest reducer (#…
Browse files Browse the repository at this point in the history
…15124)

* Eager bailout optimization should always compare to latest reducer

* queue.eagerReducer -> queue.lastRenderedReducer

This name is a bit more descriptive.

* Add test case that uses preceding render phase update
  • Loading branch information
acdlite authored Mar 16, 2019
1 parent 4162f60 commit d926936
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 15 deletions.
30 changes: 15 additions & 15 deletions packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,8 @@ type Update<S, A> = {
type UpdateQueue<S, A> = {
last: Update<S, A> | null,
dispatch: (A => mixed) | null,
eagerReducer: ((S, A) => S) | null,
eagerState: S | null,
lastRenderedReducer: ((S, A) => S) | null,
lastRenderedState: S | null,
};

export type HookType =
Expand Down Expand Up @@ -603,8 +603,8 @@ function mountReducer<S, I, A>(
const queue = (hook.queue = {
last: null,
dispatch: null,
eagerReducer: reducer,
eagerState: (initialState: any),
lastRenderedReducer: reducer,
lastRenderedState: (initialState: any),
});
const dispatch: Dispatch<A> = (queue.dispatch = (dispatchAction.bind(
null,
Expand All @@ -627,6 +627,8 @@ function updateReducer<S, I, A>(
'Should have a queue. This is likely a bug in React. Please file an issue.',
);

queue.lastRenderedReducer = reducer;

if (numberOfReRenders > 0) {
// This is a re-render. Apply the new render phase updates to the previous
// work-in-progress hook.
Expand Down Expand Up @@ -662,8 +664,7 @@ function updateReducer<S, I, A>(
hook.baseState = newState;
}

queue.eagerReducer = reducer;
queue.eagerState = newState;
queue.lastRenderedState = newState;

return [newState, dispatch];
}
Expand Down Expand Up @@ -742,8 +743,7 @@ function updateReducer<S, I, A>(
hook.baseUpdate = newBaseUpdate;
hook.baseState = newBaseState;

queue.eagerReducer = reducer;
queue.eagerState = newState;
queue.lastRenderedState = newState;
}

const dispatch: Dispatch<A> = (queue.dispatch: any);
Expand All @@ -761,8 +761,8 @@ function mountState<S>(
const queue = (hook.queue = {
last: null,
dispatch: null,
eagerReducer: basicStateReducer,
eagerState: (initialState: any),
lastRenderedReducer: basicStateReducer,
lastRenderedState: (initialState: any),
});
const dispatch: Dispatch<
BasicStateAction<S>,
Expand Down Expand Up @@ -1141,21 +1141,21 @@ function dispatchAction<S, A>(
// The queue is currently empty, which means we can eagerly compute the
// next state before entering the render phase. If the new state is the
// same as the current state, we may be able to bail out entirely.
const eagerReducer = queue.eagerReducer;
if (eagerReducer !== null) {
const lastRenderedReducer = queue.lastRenderedReducer;
if (lastRenderedReducer !== null) {
let prevDispatcher;
if (__DEV__) {
prevDispatcher = ReactCurrentDispatcher.current;
ReactCurrentDispatcher.current = InvalidNestedHooksDispatcherOnUpdateInDEV;
}
try {
const currentState: S = (queue.eagerState: any);
const eagerState = eagerReducer(currentState, action);
const currentState: S = (queue.lastRenderedState: any);
const eagerState = lastRenderedReducer(currentState, action);
// Stash the eagerly computed state, and the reducer used to compute
// it, on the update object. If the reducer hasn't changed by the
// time we enter the render phase, then the eager state can be used
// without calling the reducer again.
update.eagerReducer = eagerReducer;
update.eagerReducer = lastRenderedReducer;
update.eagerState = eagerState;
if (is(eagerState, currentState)) {
// Fast path. We can bail out without scheduling React to re-render.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1963,4 +1963,88 @@ describe('ReactHooksWithNoopRenderer', () => {
// );
});
});

it('eager bailout optimization should always compare to latest rendered reducer', () => {
// Edge case based on a bug report
let setCounter;
function App() {
const [counter, _setCounter] = useState(1);
setCounter = _setCounter;
return <Component count={counter} />;
}

function Component({count}) {
const [state, dispatch] = useReducer(() => {
// This reducer closes over a value from props. If the reducer is not
// properly updated, the eager reducer will compare to an old value
// and bail out incorrectly.
Scheduler.yieldValue('Reducer: ' + count);
return count;
}, -1);
useEffect(
() => {
Scheduler.yieldValue('Effect: ' + count);
dispatch();
},
[count],
);
Scheduler.yieldValue('Render: ' + state);
return count;
}

ReactNoop.render(<App />);
expect(Scheduler).toFlushAndYield([
'Render: -1',
'Effect: 1',
'Reducer: 1',
'Reducer: 1',
'Render: 1',
]);
expect(ReactNoop).toMatchRenderedOutput('1');

act(() => {
setCounter(2);
});
expect(Scheduler).toFlushAndYield([
'Render: 1',
'Effect: 2',
'Reducer: 2',
'Reducer: 2',
'Render: 2',
]);
expect(ReactNoop).toMatchRenderedOutput('2');
});

it('should update latest rendered reducer when a preceding state receives a render phase update', () => {
// Similar to previous test, except using a preceding render phase update
// instead of new props.
let dispatch;
function App() {
const [step, setStep] = useState(0);
const [shadow, _dispatch] = useReducer(() => step, step);
dispatch = _dispatch;

if (step < 5) {
setStep(step + 1);
}

Scheduler.yieldValue(`Step: ${step}, Shadow: ${shadow}`);
return shadow;
}

ReactNoop.render(<App />);
expect(Scheduler).toFlushAndYield([
'Step: 0, Shadow: 0',
'Step: 1, Shadow: 0',
'Step: 2, Shadow: 0',
'Step: 3, Shadow: 0',
'Step: 4, Shadow: 0',
'Step: 5, Shadow: 0',
]);
expect(ReactNoop).toMatchRenderedOutput('0');

act(() => dispatch());
expect(Scheduler).toFlushAndYield(['Step: 5, Shadow: 5']);
expect(ReactNoop).toMatchRenderedOutput('5');
});
});

0 comments on commit d926936

Please sign in to comment.