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

Eager bailout optimization should always compare to latest reducer #15124

Merged
merged 3 commits into from Mar 16, 2019

Conversation

@acdlite
Copy link
Member

commented Mar 16, 2019

Based on a bug report by @pomber

https://mobile.twitter.com/pomber/status/1106667842844401670

The issue was that the queue contains a reference to the last rendered reducer, but we weren't updating it in all cases, so sometimes it was stale, triggering the bailout incorrectly.

The original bug report (https://codesandbox.io/s/yj605043yv) had an additional bug in it that made it a bit more confusing, which is it did not specify an initial state. I did not include that part in my test case because it wasn't relevant to the actual underlying bug.

This name is a bit more descriptive.
@gaearon

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

I tried fixing it like this, and it also passed tests: 6670512. Is my fix wrong? If yes, can we add a new case that this naïve fix would fail?

@acdlite

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2019

@gaearon That's essentially the same fix except in yours doesn't account for the render phase updates path. So I think it would break if an earlier state in the same component gets a render phase update, and then a subsequent inline reducer closes over that. I'll see if I can write a test case where that would fail.

@acdlite

This comment has been minimized.

Copy link
Member Author

commented Mar 16, 2019

Added additional test that fails if reducer is not updated in the render phase updates branch.

@acdlite acdlite merged commit d926936 into facebook:master Mar 16, 2019
1 check passed
1 check passed
ci/circleci Your tests passed on CircleCI!
Details
@pomber pomber referenced this pull request Mar 16, 2019
gaearon added a commit to gaearon/react that referenced this pull request Mar 27, 2019
…acebook#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
@gaearon gaearon referenced this pull request Mar 27, 2019
Kiku-Reise added a commit to Kiku-Reise/react that referenced this pull request May 16, 2019
…acebook#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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.