Fixed useSyncExternalStoreWithSelector to update memoizedSnapshot on change
#25968
Conversation
jellevoost
changed the title
useSyncExternalStoreWithSelector to update memoizedSnapshot …useSyncExternalStoreWithSelector to update memoizedSnapshot on change
|
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! |
|
oh nice |
|
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
added
the
Resolution: Stale
|
Not stale |
|
Will mention for anyone else stumbling here that we've been applying @jellevoost's change to |
github-actions
bot
removed
the
Resolution: Stale
Looks like there's a PR implementing the same fix but with a test #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. |
github-actions
bot
added
the
Resolution: Stale
|
Still not stale |
|
@rickhanlonii as @jy14898 mentioned, that my PR also has tests for that issue. |
…n change (facebook#25968) ## Summary <!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? --> A proposed fix for the bug described in facebook#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 <!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes the user interface. How exactly did you verify that your PR solves the issue you wanted to solve? If you leave this empty, your PR will very likely be closed. -->
…n change (facebook#25968) ## Summary <!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? --> A proposed fix for the bug described in facebook#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 <!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes the user interface. How exactly did you verify that your PR solves the issue you wanted to solve? If you leave this empty, your PR will very likely be closed. -->
Summary
A proposed fix for the bug described in #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