react
react copied to clipboard
facebook
Fixed `useSyncExternalStoreWithSelector` to update memoizedSnapshot on change
Summary
A proposed fix for the bug described in https://github.com/facebook/react/issues/25967
How did you test this change?
See the issue linked above, test scenario included in the code sandbox: https://codesandbox.io/s/fervent-ives-0vm9es?file=/src/App.jsx
Unfortunately I did not figure out a way to properly unit test this issue which is why I've included the sandbox to prove that the fix works as intended. If anyone has any idea on how to add a unit test to this I would be happy to add them!
Hi @jellevoost - I'm finding the stale snapshot issue this fixes is a serious issue for our application. Did you end up using this solution in your own code, and if so, did it work well for you? I'm likely going to pick it up for our codebase but just wanted to do a bit of due diligence first in case you had any other findings since this PR was created. Thanks for figuring this out!
Hi @jellevoost - I'm finding the stale snapshot issue this fixes is a serious issue for our application. Did you end up using this solution in your own code, and if so, did it work well for you? I'm likely going to pick it up for our codebase but just wanted to do a bit of due diligence first in case you had any other findings since this PR was created. Thanks for figuring this out!
As it was quite a serious problem for me as well (leading to quite a few OOM crashes) I did include the fix and it has no longer posed any problem or any noticeable side-effect since (and there are quite a bunch of users that use it extensively). Since in my case the problem was only present in combination of react-redux (as it was using the use-sync-external-store shim) the fix was included the react-redux and patched in there. I've not yet tested v9 of react-redux in combination with react 18.2.0, but looking at the changes I think the problem still exists there (but you can no longer fix the shim included in react-redux as it relies fully on react 18 now) and you would have to patch react with this change.
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.
![github-actions[bot] avatar](https://avatars.githubusercontent.com/in/15368?v=4 "Published by a pub.dev verified publisher"){kind=small}
Will mention for anyone else stumbling here that we've been applying @jellevoost's change to use-sync-external-store using patch-package and it's been working well to resolve the memory issues we saw without any noticeable negative effects!
Unfortunately I did not figure out a way to properly unit test this issue which is why I've included the sandbox to prove that the fix works as intended. If anyone has any idea on how to add a unit test to this I would be happy to add them!
Looks like there's a PR implementing the same fix but with a test https://github.com/facebook/react/pull/27776
This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.
