[@xstate/store] Fix useSelector(…) infinite loop
#5080
Conversation
|
| @@ -0,0 +1,101 @@ | |||
| // Copied from https://github.com/facebook/react/blob/0a76aecbfa3d493c6b97998d0444f6505f5e5365/packages/use-sync-external-store/src/useSyncExternalStoreWithSelector.js | |||
There was a problem hiding this comment.
this hurts my soul, I completely miss the point of copy-pasting code just to claim the package has no redundant dependencies. Now imagine 10 libraries doing the same. As a result, the final application will have more code and not less.
There was a problem hiding this comment.
If there's a way we can fix the existing code (cc @TkDodo) then let's do that. Otherwise, I don't mind reintroducing the dependency.
There was a problem hiding this comment.
I'm not sure there is something to fix. If you create a new object or array in your selector, the comparison method must account for that. Per default, we use === to compare, so it won't work.
It would need:
const count = useSelector(
store,
(s) => s.context.count.slice(),
shallowEqual
);
There was a problem hiding this comment.
I wonder why it worked with useSyncExternalStoreWithSelector then?
| @@ -266,3 +266,35 @@ it('useActorRef (@xstate/react) should work', () => { | |||
|
|
|||
| expect(countDiv.textContent).toEqual('1'); | |||
| }); | |||
|
|
|||
| it('useSelector should not infinite loop', () => { | |||
There was a problem hiding this comment.
this is a known caveat listed in the React docs:
The store snapshot returned by getSnapshot must be immutable. If the underlying store has mutable data, return a new immutable snapshot if the data has changed. Otherwise, return a cached last snapshot.
Is it worth "patching" the problem on our side if this is just how things are "supposed" to work?
There was a problem hiding this comment.
Wouldn't zustand v5 have the same problem? The solution here is heavily inspired by how they do it:
I think the solution here would be to have the user provide a different compare method like shallow. Otherwise, yes, returning new instances will have this impact.
There was a problem hiding this comment.
To clarify, the problem here isn't that it rerenders every time; that's expected and a good use-case for providing a different compare method.
The problem is that it will cause an infinite loop:
Error: Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate. React limits the number of nested updates to prevent infinite loops.
EDIT: Okay, I see that this is a caveat in Zustand as well: https://zustand.docs.pmnd.rs/migrations/migrating-to-v5#requiring-stable-selector-outputs
There was a problem hiding this comment.
To clarify, the problem here isn't that it rerenders every time
The problem is that it will cause an infinite loop:
You are saying that what is a problem isn't a problem or that what isn't a problem is a problem :D
There was a problem hiding this comment.
When this happens with the current code React even prints a warning about it:
The result of getSnapshot should be cached to avoid an infinite loop
The
useSelector(…)hook will no longer result in an infinite loop when an object-like value is returned from the selector function.cc. @TkDodo - I had to bring back in
useSyncExternalStoreWithSelector(copy-pasted, no dependencies), but if you have any better ideas for solving this, I'm open to a simpler solution.