Eager bailouts only for use state #18241
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…rocessed incorrectly on subsequent rerenders
Andarist
commented
Mar 6, 2020
| @@ -3173,6 +3123,59 @@ function loadModules({ | |||
| expect(ReactNoop).toMatchRenderedOutput('5'); | |||
| }); | |||
|
|
|||
| it('should not store dispatched items in the queue (TODO: better description)', () => { | |||
There was a problem hiding this comment.
I'm not sure how to best describe this test as it sort of tests the mechanism that is no longer here (eager bailouts for use reducer).
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 897945f:
|
Details of bundled changes.Comparing: 4027f2a...897945f react-dom
react-native-renderer
react-art
react-test-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (stable) |
Details of bundled changes.Comparing: 4027f2a...897945f react-art
react-native-renderer
react-dom
react-test-renderer
react-reconciler
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (experimental) |
Andarist
commented
Mar 6, 2020
| } | ||
| } | ||
|
|
||
| function setState<S>( |
There was a problem hiding this comment.
For now this is being split from the dispatchAction but they both share pretty much the same implementation - besides the fact that dispatchAction has no eager bailout implemented.
Both could be collapsed into a single function and the eager bailout could only be attempted for reducer === basicStateReducer but to do that I would have to bring back keeping track of lastRenderedReducer.
Let me know which approach do you prefer.
jameskip
approved these changes
Apr 1, 2020
|
So we've been looking at this. It's probably the right thing to do. But it's a scary change. So we'd want it behind a flag. The current implementation in this PR would be hard to put behind a flag. It would also be hard to maintain two versions in one place. We plan to fork the reconciler with a few other breaking and more ambitious changes. So we'll probably fix this as a part of that refactor. Thank you for starting this work though (and for flagging the issue in the first place). It's been really helpful. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
fixes #15088 closes #15198
I've stumbled upon an edge case where bailed out actions were kept in the update queue and could be applied using the newly rendered reducer during future rerender. This can be observed here - https://codesandbox.io/s/silly-paper-22org . Just click the disable button, click the increment button multiple times and click the disable button once again. The counter will jump to the number of times you have clicked on the incrementing button while it really should still show 0.
@sebmarkbage has suggested ditching the eager bailout optimization for the useReducer case altogether here and @gaearon has recently encouraged me to just follow this suggestion.
@sebmarkbage has also suggested that "We should also forbid suspending in a setState reducer.", but I'm not sure what that means exactly and how it should be approached so any tips regarding that would be highly appreciated.
As to the implementation - I've managed to remove the need to store "lastRenderedReducer" at all and also I've decided to only append updates to the queue if the rerender is scheduled. In the current implementation on the master branch, the queue was slightly prone to a minor memory leak because theoretically, it was possible to append bailed out updates until the next rerender and that, theoretically, could never happen. This is not a real-life scenario, but it still seems better to avoid appending to the queue unnecessarily to avoid going through those updates during the next render.
Test Plan
I've run tests, lint & flow dom tasks before sending this as PR.