stale closure problem with select option

[TkDodo]

Describe the bug

The select option function is only executed if data (the input to that function) changes. We can see this here, the previous select result is used if the result data hasn't changed:

https://github.com/tannerlinsley/react-query/blob/2de2a0dc322fed60648b4294a613189f967f1f91/src/core/queryObserver.ts#L378-L389

But if you have any other dependency from a closure to that select function, it will not be executed even though a "dependency" to it changes.

To Reproduce

https://codesandbox.io/s/serverless-cdn-3besp?file=/src/index.js

1. Click on the button, which will increment the count
2. count is used in the select function, but it is not executed

Expected behavior
select to be executed on every render

Desktop:

• OS: macOS Catalaina
• Browser: chrome
• Version: 3.5.12

Additional Context

Another interesting fact seems to be that we are dependent on the data of the observer (this.currentResult), which means that if you unmount your component, it will execute the selector again, even though we still have data in the cache. See https://codesandbox.io/s/magical-currying-jo0j9?file=/src/index.js

• click the Inc button, nothing happens
• click Remount -> select now executes.

---

This can happen when you do route changes for example. It's a bit undeterministic when the selector function actually runs, so I think it would be better to err on the side of caution and always re-run it. The optimization of not running the selector function in some situations seems a bit premature, If I'm not mistaken, redux also runs the selector on every render, and the suggested solution would be to memoize selectors yourself, e.g. with reselect. Usually, running the selectors is not very expensive.

@boschni FYI

[boschni]

It kind of depends on what the purpose of select is. Currently it is designed to only select/transform parts of the cached query data. It runs when the cached query data changes and it only triggers a re-render when the selected result is different. If this optimization is not needed, or if the transformation needs external dependencies, then the data can also be transformed in the render function with useMemo or regular functions. The select result is tied to an observer because each observer might need different parts of the query data.

[TkDodo]

Understood. I think at least we should document this behavior, because people are a bit confused :)

[boschni]

It is possible to support external dependencies with a dependency array like selectDeps. But it's yet another option and does make things a bit more complex.

[TkDodo]

How about always running it if data changed or the reference to the function itself changed? It would always run with inline functions, but people could memoize it with useMemo

[boschni]

It's an option but it would degrade performance for most users as they will probably not memoize the function.

This means that for each render:

• The select function needs to be executed (which could be heavy).
• The result of it then needs to be deeply compared (could also be heavy).
• If the result is different, the observer state needs to be updated, which would trigger another render.

[TkDodo]

The select function needs to be executed (which could be heavy).

I would not think that most select functions are heavy. if they are, users can memoize them on their side

The result of it then needs to be deeply compared (could also be heavy).

if the result is different from the last result, yes. if users memoize the select function, we could rely on referential identity.

If the result is different, the observer state needs to be updated, which would trigger another render.

isn't this now the case as well?

if the transformation needs external dependencies, then the data can also be transformed in the render function with useMemo

I guess one problem is that we try to have two features with one function. There is a difference between "selecting data" and "transforming data". Transformation can be heavy and only depends on data, while selecting is usually fast (just pick a prop or so), but can also have external dependencies.

What we currently cannot do is for example: Have an api that only returns a list of things, but I want a detail view that just selects one entity from the list, and only re-renders if that item changes, e.g.:

function useTodo(id) {
    return useQuery(
        'todos',
        fetchAllTodos,
        {
            select: data => data.find(todo => todo.id === id)
        }
    )
}

now this doesn't work because if we get the same data back, but our id changes, we won't run the select function. If I go ahead and do this with useMemo, I cannot bail out of rendering if the list changes, but my own id did not change:

function useTodo(id) {
    const queryInfo = useQuery(
        'todos',
        fetchAllTodos,
    )

    return {
        ...queryInfo,
        data: useMemo(data => data.find(todo => todo.id === id), [data, id])
    }
}

I think most people would expect select to always run, because that's the concept of selectors that other libraries (redux, zustand) also have....

[boschni]

The current select implementation is highly optimized out of the box, but it does not support dynamic external dependencies. When adding an option like selectDeps, this would remain the case, but it also allows users to specify dependencies which would be shallowly compared on each render. When executing selectors on each render, users become fully responsible for any performance optimization and it's easy to overlook.

In case of redux, selectors should only run when either the state or the function reference change. As people mostly use function references like useSelector(getSomeState), they also benefit from out of the box optimization.

Edit: I do see the use case and the current behavior could potentially result in bugs if people include external dependencies in their selectors. It was not designed with this in mind, but if we do want to support it, then either we need to drop the optimization (could result in degraded performance) or introduce a dependency array (could still result in bugs).

[TkDodo]

selectDeps doesn't seem like such a good idea imo. The dependencies only work for things like useMemo and useEffect because there are lint rules for it, and it's directly coupled to the function call. If we add that as a separate prop, it might easily get out of sync with the real implementation (I guess thats what you meant with "could still result in bugs").

I get that select was meant as "select something from data", but yeah, there are other use-cases as well. Not sure what the best way forward would be.

I still feel that we could try running the select function every time referential identity of the select function changes or data changes. It would give people the chance to optimize with useMemo or by extracting the function (select: getSomeState) if they run into perf issues because of inline selectors, and it would allow external dependencies.
The heaviest part is probably the deep compare - question is if we need to do that with the result of the select function? If people just do:

select: data => data.something

a referential comparison would be enough, because we have already deeply compared data, so just picking something from data will be stable. Of course, if you create a new object in your selector, it won't work - but this is generally discouraged with selectors I believe (in redux et al, because useSelector also triple equal compares per default).

such a change would probably be "too breaking" for a minor release though, maybe we should think more about a path for v4?

[tannerlinsley]

My vote is a breaking change later on in v4 of either:

• Do not further tie the configuration to React. Remove the select feature and document how to "select" in userland with memoization.
• Remove the current implementation of select and redesign the functionality at a higher level of control that deliberately supports memoization. This might even fit into the new model I was dreaming up for v4 in discord @TkDodo @boschni.
• We always could fully adopt the React methodology here and design the API to expect a stable function which it can shallow compare between renders
• Something else

Either way, I think we're definitely looking at a v4 design challenge.

[boschni]

I do think it would be good to fix this in v3. Currently people will just include external dependencies in the select function, and it'll seem to work fine at first sight, until a dependency changes for some reason. This could result in buggy applications, and the only way to fix it, is to replace the select function with something like useMemo, which degrades the DX and performance.

Triggering the select function on each render would be the safest solution, but will degrade performance for some users. Adding selectDeps is less safe, but does provide a solution for the problem and the existence of the property might trigger users to think about it. I would be fine with either option.

[tannerlinsley]

I'm fine with either as well, but lean towards the simplest solution of just running the function every time. If further issues arise, we can certainly handle them as they come, but they may not.

[TkDodo]

I'm fine with either as well, but lean towards the simplest solution of just running the function every time. If further issues arise, we can certainly handle them as they come, but they may not.

I think it's also quite simple to extend this to referential stability of the select function, which means it would run always if you use an inline select function (most usages I presume), but you could optimize it by extracting it to a static function if its expensive but it has no dependencies, or optimize with useCallback on the callside. Of course, it would always need to run if data changes.

[johnsonav1992]

I know this thread is super old...but was select ever updated to rerun even if the data itself did not change? I currently have a use case where I need a select fb that can rerun on the same data as many times as needed (for FE filtering purposes). Please let me know @TkDodo @tannerlinsley ! Thank you!