Do not keep bailouted actions which might lead to reducer computing wrong value on update later #15198
Conversation
Andarist
force-pushed
the
fix/stale-bailouts-reused
branch
2 times, most recently
from
5ae4b7c to
f39995f
Compare
Andarist
force-pushed
the
fix/stale-bailouts-reused
branch
from
f39995f to
d2a0516
Compare
…rong value on update later
|
This is an interesting bug, but I don't think it's the right fix. I think I have a better one but I'm on my phone right now so I'll follow up later, or on Monday. |
|
Might not be the best, although the fix is pretty straightforward and doesn't seem like a hacky solution. If you have any thoughts on how this could be improved I'm all ears - I'd love to work on this if possible. Enjoy your weekend! |
|
Test case for you to consider: A parent and a child are both updated in the same batch. The child bails out early, but the parent doesn’t. The parent re-renders and passes a new prop to the child, which closes over the new prop in its reducer. What do you think should happen? |
… parent rerenders with new props
|
Didn't consider that! I assume that React's assumption is that scheduled work in a batch should execute top-down, therefore eager bailout lower in the tree should get ignored (or rather - the action should get processed by the reducer in the render phase if a parent has provided new props to a child which enable the child's state update). I've added a failing test case for this 👍 Do you have any possible solutions in mind? I could try to explore this fully on my own but for sure it would take me way longer as I'm not that familiar with the codebase here. I would appreciate any pointers so I could continue working on a fix. |
|
@acdlite any tips on how I could implement proper fix for this? |
|
@acdlite friendly 🏓 would love to explore fixing this but atm I'm not sure what the appropriate approach to tackle this would be |
|
I think @acdlite would probably have already replied or fixed this if he had a strategy in mind. We appreciate the help but I think you might need to try something and look at in more depth to form a proposal if you want to move it forward. |
|
I think we should probably disable this optimization for useReducer and split out the implementation of useState to a separate implementation since that can't change the reducer it's always safe to precompute the state. We should also forbid suspending in a setState reducer. This simplifies the implementation in terms of number of fields we need to keep around and should fix this issue by always rerendering reducers. |
|
@sebmarkbage let me know if I could help with that - the codebase is complex though and I probably would not be able to make those changes on my own without some guidance from the React team. |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution. |
|
The issue still persists: https://codesandbox.io/s/silly-paper-22org |
|
@Andarist do you want to take a shot at the above suggestion? |
|
@gaearon yes! but frankly - I'm not sure what is being exactly suggested right now. Should I "simply" remove this optimization from useReducer and provide a new implementation for useState (not depending on useReducer)? Ono concern I have about removing this optimization is that we can't know if a reducer is static or dynamic, so if we remove this entirely we'd have to always schedule a full component render for each dispatched action. Is that correct? |
Maybe let's start with this and then figure out if we need to do something else. |
|
Closing this as it is supposed to be fixed with other reconciler stuff as per this comment |
* Fork dispatchAction for useState/useReducer * Remove eager bailout from forked dispatchReducerAction, update tests * Update eager reducer/state logic to handle state case only * sync reconciler fork * rename test * test cases from #15198 * comments on new test cases * comments on new test cases * test case from #21419 * minor tweak to test name to kick CI
* Fork dispatchAction for useState/useReducer * Remove eager bailout from forked dispatchReducerAction, update tests * Update eager reducer/state logic to handle state case only * sync reconciler fork * rename test * test cases from facebook#15198 * comments on new test cases * comments on new test cases * test case from facebook#21419 * minor tweak to test name to kick CI
This is a fix for what I think is a bug which I've reported in #15088
The issue might be observed here https://codesandbox.io/s/n97q1o6nq4 . Just increment the counter -> disable the whole thing -> increment few times (those are going to bail out) -> enable the counter -> observe a counter incrementing by the amount of actions which have bailouted previously.