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

Don't discard render phase state updates with the eager reducer optimization #14852

Merged
merged 3 commits into from Feb 19, 2019

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Feb 14, 2019

Reproduces #14849 in 2 out of 3 cases (with hoisted reducer and useState but not with inline reducer). Points to a bug in eager update logic. Not sure where it is yet.

@gaearon gaearon changed the title Add test cases for setState(fn) + render phase updates Don't discard render phase state updates with the eager reducer optimization Feb 14, 2019
@gaearon
Copy link
Collaborator Author

gaearon commented Feb 14, 2019

Pushed a potential fix in 6f44db2. I don't really know if it's correct. It would really help to have a written description of how the eager optimization works and what assumptions it makes.

Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good test cases

@acdlite
Copy link
Collaborator

acdlite commented Feb 19, 2019

I wrote a partial description of how the optimization is supposed to work here. Maybe we can add more to it:

// 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) {
let prevDispatcher;
if (__DEV__) {
prevDispatcher = ReactCurrentDispatcher.current;
ReactCurrentDispatcher.current = InvalidNestedHooksDispatcherOnUpdateInDEV;
}
try {
const currentState: S = (queue.eagerState: any);
const eagerState = eagerReducer(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.eagerState = eagerState;
if (is(eagerState, currentState)) {
// Fast path. We can bail out without scheduling React to re-render.
// It's still possible that we'll need to rebase this update later,
// if the component re-renders for a different reason and by that
// time the reducer has changed.

Specifically, should add that eagerReducer and eagerState represent the values during the last render attempt. If there's no pending work, then we know those are equal to the current values. Maybe we can think of a better name for those, too.

@gaearon gaearon merged commit c506ded into facebook:master Feb 19, 2019
@gaearon gaearon deleted the bug-state branch February 19, 2019 18:40
acdlite pushed a commit to acdlite/react that referenced this pull request Feb 20, 2019
…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
Deraen added a commit to cljsjs/packages that referenced this pull request Mar 2, 2019
## 16.8.2 => 16.8.3

### React DOM

* Fix a bug that caused inputs to behave incorrectly in UMD builds. ([@gaearon](https://github.com/gaearon) in [#14914](facebook/react#14914))
* Fix a bug that caused render phase updates to be discarded. ([@gaearon](https://github.com/gaearon) in [#14852](facebook/react#14852))

### React DOM Server
* Unwind the context stack when a stream is destroyed without completing, to prevent incorrect values during a subsequent render. ([@overlookmotel](https://github.com/overlookmotel) in [#14706](facebook/react#14706))
Deraen added a commit to cljsjs/packages that referenced this pull request Mar 2, 2019
## 16.8.2 => 16.8.3

### React DOM

* Fix a bug that caused inputs to behave incorrectly in UMD builds. ([@gaearon](https://github.com/gaearon) in [#14914](facebook/react#14914))
* Fix a bug that caused render phase updates to be discarded. ([@gaearon](https://github.com/gaearon) in [#14852](facebook/react#14852))

### React DOM Server
* Unwind the context stack when a stream is destroyed without completing, to prevent incorrect values during a subsequent render. ([@overlookmotel](https://github.com/overlookmotel) in [#14706](facebook/react#14706))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants