A possible idea towards fixing useSelector tearing in Concurrent Mode #1509
Conversation
|
Deploy preview for react-redux-docs ready! Built with commit 0768b45 |
There was a problem hiding this comment.
without having looked at the failing tests, just reading the code I can see two potential staleness issues (see my inline comments in the files). overall this seems like a very interesting approach. unfortunately I haven't really used (or though about) neither react nor react-redux recently, so I am not that deep in the code
src/hooks/useSelector.js
Outdated
| selectedState = latestSelectedState.current | ||
| if (selector !== state.selector || state.subscriptionCallbackError) { | ||
| const newSelectedState = selector(state.storeState) | ||
| if (!equalityFn(newSelectedState, selectedState)) { |
There was a problem hiding this comment.
potential staleness issue here: if the selector changes, but the result does not, the selector does not get updated in the state, i.e. the callback from the effect uses a stale selector
There was a problem hiding this comment.
Thanks for the comments. Pretty much appreciated.
I knew the staleness, but yeah, this is critical.
state = 3;
selector1 = state => state - 2;
selector2 = state => state / 3;
selector1(state) === 1
selector2(state) === 1Hmm, I don't think I have any solutions to this now, except for stable selectors (useCallback) which is a breaking change.
src/hooks/useSelector.js
Outdated
| try { | ||
| newSelectedState = prevState.selector(newStoreState) | ||
|
|
||
| if (equalityFn(newSelectedState, prevState.selectedState)) { |
There was a problem hiding this comment.
potential staleness issue here: if the store state changes, but the selected state does not, then in line 30 you may be using a stale state if the component gets rendered due to an outside effect
There was a problem hiding this comment.
Oh, this one I missed. Absolutely.
Only the solution I can think of again is to useCallback. OK, that was difficult.
|
Because this doesn't work as is, this is just an idea for possible solutions. Let me change the title. At this moment, I have no idea to complete this without a breaking change. |
dai-shi
changed the title
|
Actually, I have an idea to combine the "cheat mode" I tried in #1505. This one seems better. I would like someone to review and/or try. |
| if (latestSubscriptionCallbackError.current) { | ||
| err.message += `\nThe error may be correlated with this previous error:\n${latestSubscriptionCallbackError.current.stack}\n\n` | ||
| let selectedState = state.selectedState | ||
| if (state.selector !== selector) { |
There was a problem hiding this comment.
is this whole if even required anymore? the reducer above always uses the latest selector during rendering, which means that state.selectedState should already have been computed with the latest selector, no? tbh, I am not sure I fully understand the "cheat mode" yet and when exactly the reducer is called with what values, so I may be wrong here
There was a problem hiding this comment.
I happened to notice a bug, and fixed. 0768b45
It probably better explains why this if is required.
The rule of thumb is never read store state in render. Update it through React state.
Honestly, I don't fully understand it either and it behaves a bit differently from what I expected: The reducer seems to be called before render with various values in CM. That's probably why this if is required.
This actually fixes the tearing issue as far as my tool goes. I'd admit that there might still be edge cases not covered. Hope someone could help it here.
|
Before Mark asking, here's the benchmark result. Of course there's a trade-off. |
|
https://gist.github.com/bvaughn/054b82781bec875345bd85a5b1344698 |
proud to see that finally my ideas were right useSubscription also read values while render, and re-enque a setState, #1455 (comment) anyway because they are the "core team" they can adop an hard definetively solution |
|
I'm going to close this PR, because it's no longer intended to merge in. We will be discussing a new implementation with useMutableSource in #1351. However, the implementation in this PR is interesting and worth learning. |
This is partly for #1351.
This tries to take the same approach as use-subscription by React team.
It's not exactly the same, because in react-redux, a selector doesn't have to be stable.
(Also noticed, equalityFn in callback is read from the closure.)
I don't fully understand #1455, but the approach is probably different.
There are three test cases failing. two of them I know why. the last one I'm not sure.
I've tested it with my tool, but ideally we should add a test like the one in use-subscription.
Do we take it seriously?
Those who might be interested: @markerikson @MrWolfZ @eddyw @salvoravida