Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: Stale selectors keep old store snapshots alive in useSyncExternalStoreWithSelector #25967

Open
jellevoost opened this issue Jan 6, 2023 · 4 comments
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@jellevoost
Copy link

Within useSyncExternalStoreWithSelector there is currently a bug that will keep old references to the used store alive if you use an immutable store in combination with selectors that always result in the same result. This can lead to excessive memory usage while this is not needed. I've noticed this behavior in combination with react-redux, but also managed to reproduce it without react-redux to figure out exactly what was going on. (I've reported this at the react-redux repo as well reduxjs/react-redux#1981)

React version: 18.2.0

Steps To Reproduce

Since the reproduction is fairly complicated I've created a sandbox with details on how to reproduce including a minimal working sample that also includes the reproduction steps within that exact example.

But in summary, it is reproducible using the following steps:

  1. You will need an immutable store that you will use with your selector (new copy every store update)
  2. You will need a stable custom isEqual function and selector that are not created in-line
  3. The store needs some property (e.g. a string) that is easily found within memory snapshots. (It helps to include a timestamp within this property that updates on store change)
  4. You will need to print a value from the store within your main component, to show the most recent result of the store.
  5. You will need a component that uses a stable selector to select a stale value (something that never changes) from the store
  6. Next up you need to update the store and add an additional copy of the previous component (the one created in step 5.)
  7. Repeat this a few times
  8. Take a memory snapshot and notice there are multiple copies of the store present in memoizedSnapshots of the different components. You can see this by searching on the property you defined in step 3.

Link to code example:
https://codesandbox.io/s/fervent-ives-0vm9es?file=/src/App.jsx

The current behavior

  • Whenever the result of getSnapshot() is changed, but the result of the selector() has not, the memoized reference to the old result of getSnapshot() is not updated, resulting in unnecessary copies of the store used alive. Whenever you have a fairly large store that is shared between quite a bunch of selectors, especially with components that are mounted at a later timestamp and use selectors that have stale data, you could end up with an ever increasing amount of store references resulting in high memory usage.

The expected behavior

  • Whenever the result of getSnapshot() is changed, but the result of the selector() has not, the memoized reference to the old result of getSnapshot() is updated correctly, preventing unnecessary copies from being kept alive. This should not impact the behavior of useSyncExternalStoreWithSelector but should/can reduce the memory footprint of applications using this.
@jellevoost jellevoost added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Jan 6, 2023
@jellevoost
Copy link
Author

I've included the fix as a PR that was also included in the codesandbox that should resolve the issue.

Copy link

github-actions bot commented Apr 9, 2024

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

@github-actions github-actions bot added the Resolution: Stale Automatically closed due to inactivity label Apr 9, 2024
@talaikis
Copy link

talaikis commented Apr 9, 2024

The real solution is to drop redux.

@jellevoost
Copy link
Author

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

This issue is still present

@github-actions github-actions bot removed the Resolution: Stale Automatically closed due to inactivity label Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

2 participants