Skip to content

Commit

Permalink
Don't discard render phase state updates with the eager reducer optim…
Browse files Browse the repository at this point in the history
…ization (facebook#14852)

* Add test cases for setState(fn) + render phase updates

* Update eager state and reducer for render phase updates

* Fix a newly firing warning
  • Loading branch information
gaearon authored and acdlite committed Feb 20, 2019
1 parent 3e55560 commit 619cdfc
Show file tree
Hide file tree
Showing 3 changed files with 76 additions and 2 deletions.
4 changes: 3 additions & 1 deletion packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -607,7 +607,6 @@ function updateReducer<S, I, A>(
}

hook.memoizedState = newState;

// Don't persist the state accumlated from the render phase updates to
// the base state unless the queue is empty.
// TODO: Not sure if this is the desired semantics, but it's what we
Expand All @@ -616,6 +615,9 @@ function updateReducer<S, I, A>(
hook.baseState = newState;
}

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

return [newState, dispatch];
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -669,6 +669,76 @@ describe('ReactHooks', () => {
}).toThrow('is not a function');
});

it('does not forget render phase useState updates inside an effect', () => {
const {useState, useEffect} = React;

function Counter() {
const [counter, setCounter] = useState(0);
if (counter === 0) {
setCounter(x => x + 1);
setCounter(x => x + 1);
}
useEffect(() => {
setCounter(x => x + 1);
setCounter(x => x + 1);
}, []);
return counter;
}

const root = ReactTestRenderer.create(null);
ReactTestRenderer.act(() => {
root.update(<Counter />);
});
expect(root).toMatchRenderedOutput('4');
});

it('does not forget render phase useReducer updates inside an effect with hoisted reducer', () => {
const {useReducer, useEffect} = React;

const reducer = x => x + 1;
function Counter() {
const [counter, increment] = useReducer(reducer, 0);
if (counter === 0) {
increment();
increment();
}
useEffect(() => {
increment();
increment();
}, []);
return counter;
}

const root = ReactTestRenderer.create(null);
ReactTestRenderer.act(() => {
root.update(<Counter />);
});
expect(root).toMatchRenderedOutput('4');
});

it('does not forget render phase useReducer updates inside an effect with inline reducer', () => {
const {useReducer, useEffect} = React;

function Counter() {
const [counter, increment] = useReducer(x => x + 1, 0);
if (counter === 0) {
increment();
increment();
}
useEffect(() => {
increment();
increment();
}, []);
return counter;
}

const root = ReactTestRenderer.create(null);
ReactTestRenderer.act(() => {
root.update(<Counter />);
});
expect(root).toMatchRenderedOutput('4');
});

it('warns for bad useImperativeHandle first arg', () => {
const {useImperativeHandle} = React;
function App() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,9 @@ describe('ReactHooksWithNoopRenderer', () => {

// Test that it works on update, too. This time the log is a bit different
// because we started with reducerB instead of reducerA.
counter.current.dispatch('reset');
ReactNoop.act(() => {
counter.current.dispatch('reset');
});
ReactNoop.render(<Counter ref={counter} />);
expect(ReactNoop.flush()).toEqual([
'Render: 0',
Expand Down

0 comments on commit 619cdfc

Please sign in to comment.