Eager bailout optimization should always compare to latest reducer (#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
Andrew Clark committed
Mar 15, 2019 at 19:03 UTC
d926936f0b5ebe160819717924d7a124c2f65471
2 files changed
+99
-15
packages/react-reconciler/src/ReactFiberHooks.js
+15
-15
@@ -89,8 +89,8 @@ type Update<S, A> = {
89
type UpdateQueue<S, A> = {
90
last: Update<S, A> | null,
91
dispatch: (A => mixed) | null,
92
- eagerReducer: ((S, A) => S) | null,
93
- eagerState: S | null,
92
+ lastRenderedReducer: ((S, A) => S) | null,
93
+ lastRenderedState: S | null,
94
};
95
96
export type HookType =
@@ -603,8 +603,8 @@ function mountReducer<S, I, A>(
603
const queue = (hook.queue = {
604
last: null,
605
dispatch: null,
606
- eagerReducer: reducer,
607
- eagerState: (initialState: any),
606
+ lastRenderedReducer: reducer,
607
+ lastRenderedState: (initialState: any),
608
});
609
const dispatch: Dispatch<A> = (queue.dispatch = (dispatchAction.bind(
610
null,
@@ -627,6 +627,8 @@ function updateReducer<S, I, A>(
627
'Should have a queue. This is likely a bug in React. Please file an issue.',
628
);
629
630
+ queue.lastRenderedReducer = reducer;
631
+
632
if (numberOfReRenders > 0) {
633
// This is a re-render. Apply the new render phase updates to the previous
634
// work-in-progress hook.
@@ -662,8 +664,7 @@ function updateReducer<S, I, A>(
664
hook.baseState = newState;
665
}
666
665
- queue.eagerReducer = reducer;
666
- queue.eagerState = newState;
667
+ queue.lastRenderedState = newState;
668
669
return [newState, dispatch];
670
}
@@ -742,8 +743,7 @@ function updateReducer<S, I, A>(
743
hook.baseUpdate = newBaseUpdate;
744
hook.baseState = newBaseState;
745
745
- queue.eagerReducer = reducer;
746
- queue.eagerState = newState;
746
+ queue.lastRenderedState = newState;
747
}
748
749
const dispatch: Dispatch<A> = (queue.dispatch: any);
@@ -761,8 +761,8 @@ function mountState<S>(
761
const queue = (hook.queue = {
762
last: null,
763
dispatch: null,
764
- eagerReducer: basicStateReducer,
765
- eagerState: (initialState: any),
764
+ lastRenderedReducer: basicStateReducer,
765
+ lastRenderedState: (initialState: any),
766
});
767
const dispatch: Dispatch<
768
BasicStateAction<S>,
@@ -1141,21 +1141,21 @@ function dispatchAction<S, A>(
1141
// The queue is currently empty, which means we can eagerly compute the
1142
// next state before entering the render phase. If the new state is the
1143
// same as the current state, we may be able to bail out entirely.
1144
- const eagerReducer = queue.eagerReducer;
1145
- if (eagerReducer !== null) {
1144
+ const lastRenderedReducer = queue.lastRenderedReducer;
1145
+ if (lastRenderedReducer !== null) {
1146
let prevDispatcher;
1147
if (__DEV__) {
1148
prevDispatcher = ReactCurrentDispatcher.current;
1149
ReactCurrentDispatcher.current = InvalidNestedHooksDispatcherOnUpdateInDEV;
1150
}
1151
try {
1152
- const currentState: S = (queue.eagerState: any);
1153
- const eagerState = eagerReducer(currentState, action);
1152
+ const currentState: S = (queue.lastRenderedState: any);
1153
+ const eagerState = lastRenderedReducer(currentState, action);
1154
// Stash the eagerly computed state, and the reducer used to compute
1155
// it, on the update object. If the reducer hasn't changed by the
1156
// time we enter the render phase, then the eager state can be used
1157
// without calling the reducer again.
1158
- update.eagerReducer = eagerReducer;
1158
+ update.eagerReducer = lastRenderedReducer;
1159
update.eagerState = eagerState;
1160
if (is(eagerState, currentState)) {
1161
// Fast path. We can bail out without scheduling React to re-render.
packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js
+84
@@ -1963,4 +1963,88 @@ describe('ReactHooksWithNoopRenderer', () => {
1963
// );
1964
});
1965
});
1966
+
1967
+ it('eager bailout optimization should always compare to latest rendered reducer', () => {
1968
+ // Edge case based on a bug report
1969
+ let setCounter;
1970
+ function App() {
1971
+ const [counter, _setCounter] = useState(1);
1972
+ setCounter = _setCounter;
1973
+ return <Component count={counter} />;
1974
+ }
1975
+
1976
+ function Component({count}) {
1977
+ const [state, dispatch] = useReducer(() => {
1978
+ // This reducer closes over a value from props. If the reducer is not
1979
+ // properly updated, the eager reducer will compare to an old value
1980
+ // and bail out incorrectly.
1981
+ Scheduler.yieldValue('Reducer: ' + count);
1982
+ return count;
1983
+ }, -1);
1984
+ useEffect(
1985
+ () => {
1986
+ Scheduler.yieldValue('Effect: ' + count);
1987
+ dispatch();
1988
+ },
1989
+ [count],
1990
+ );
1991
+ Scheduler.yieldValue('Render: ' + state);
1992
+ return count;
1993
+ }
1994
+
1995
+ ReactNoop.render(<App />);
1996
+ expect(Scheduler).toFlushAndYield([
1997
+ 'Render: -1',
1998
+ 'Effect: 1',
1999
+ 'Reducer: 1',
2000
+ 'Reducer: 1',
2001
+ 'Render: 1',
2002
+ ]);
2003
+ expect(ReactNoop).toMatchRenderedOutput('1');
2004
+
2005
+ act(() => {
2006
+ setCounter(2);
2007
+ });
2008
+ expect(Scheduler).toFlushAndYield([
2009
+ 'Render: 1',
2010
+ 'Effect: 2',
2011
+ 'Reducer: 2',
2012
+ 'Reducer: 2',
2013
+ 'Render: 2',
2014
+ ]);
2015
+ expect(ReactNoop).toMatchRenderedOutput('2');
2016
+ });
2017
+
2018
+ it('should update latest rendered reducer when a preceding state receives a render phase update', () => {
2019
+ // Similar to previous test, except using a preceding render phase update
2020
+ // instead of new props.
2021
+ let dispatch;
2022
+ function App() {
2023
+ const [step, setStep] = useState(0);
2024
+ const [shadow, _dispatch] = useReducer(() => step, step);
2025
+ dispatch = _dispatch;
2026
+
2027
+ if (step < 5) {
2028
+ setStep(step + 1);
2029
+ }
2030
+
2031
+ Scheduler.yieldValue(`Step: ${step}, Shadow: ${shadow}`);
2032
+ return shadow;
2033
+ }
2034
+
2035
+ ReactNoop.render(<App />);
2036
+ expect(Scheduler).toFlushAndYield([
2037
+ 'Step: 0, Shadow: 0',
2038
+ 'Step: 1, Shadow: 0',
2039
+ 'Step: 2, Shadow: 0',
2040
+ 'Step: 3, Shadow: 0',
2041
+ 'Step: 4, Shadow: 0',
2042
+ 'Step: 5, Shadow: 0',
2043
+ ]);
2044
+ expect(ReactNoop).toMatchRenderedOutput('0');
2045
+
2046
+ act(() => dispatch());
2047
+ expect(Scheduler).toFlushAndYield(['Step: 5, Shadow: 5']);
2048
+ expect(ReactNoop).toMatchRenderedOutput('5');
2049
+ });
2050
});