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

selectFromResult runs on any change to the store #4028

Closed
thisjeremiah opened this issue Dec 31, 2023 · 3 comments
Closed

selectFromResult runs on any change to the store #4028

thisjeremiah opened this issue Dec 31, 2023 · 3 comments
Milestone

Comments

@thisjeremiah
Copy link
Contributor

Problem

I've noticed that the selectFromResult callback in useQuery seems to be running any time any data in the store changes.

I'm trying to understand if this behavior is necessary.

Expected Behavior

I'd prefer selectFromResult to only run when the relevant data changes (on changes to loading state or the response data).

Benefit

The primary benefit of improving this behavior is that if someone (intentionally or accidentally) returns an unstable response from selectFromResult, the component will only re-render twice for every request that gets made (isLoading is set to true then false).

Reproduction

I have a simple sandbox that illustrates this behavior here.

I intentionally return an unstable reference in selectFromResult for useQuery B to show how many times selectFromResult is running. (The code sandbox was not reliably emitting console logs.)

How to fix

The implementation of selectFromResult is here.

I wasn't clever enough to figure out a way to fix it myself, but my hope is that the selectFromResult selector can be modified to only run when the relevant store data changes.

Note

I noticed there are a couple of existing issues that may be relevant to the discussion:

I created a separate issue because I'm specifically looking to understand why this behavior is happening and whether it's possible to improve it.

@phryneas
Copy link
Member

I believe this change could remedy that - could you give it a try and report back?

@thisjeremiah
Copy link
Contributor Author

@phryneas That works! 🎉

I don't have a way of showing this working in my sandbox, but I do have a vite reproduction with the patch here.

I don't think there any regressions in the testing suite for the fix here. (tested tests locally).

Is this a change that you're looking to make?

@phryneas
Copy link
Member

phryneas commented Jan 1, 2024

It's on my list, but honestly I have no idea when I could get to it.

If you could open a PR and maybe add a test to the test suite that would have failed with the old behaviour, it would be very welcome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants