selectFromResult should only re-invoke the selector if it has changed

[kkirby]

For example, when using selectFromResult to select a specific post via ID from an array of posts, the selector function should not be executed if the post ID did not change.

Basically, memoize the result unless the function has changed, in which case, re-run the function.

function MyComponent({ postID }) {
  const selector = React.useCallback(({data}) => {
    return {
      data: data != null ? data.items.find(x => x.id === postID) : null,
    };
  }, [postID]);
  const { data, error, isLoading } = useGetExampleDataQuery(undefined, {
    selectFromResult: selector,
  });

This may be completely against the intended usage of selectFromResult, but I have seen code in the wild that uses selectFromResult this way.

[EskiMojo14]

what if the post being selected changed? how would you know without re-invoking the selector?

[kkirby]

If the data it depends on (the response data) changes, then of course the function should be called again.

[phryneas]

I might be missing something, but that's pretty much how it works?

Either upstream data (or loading states, other things that selectFromResult can work on) changes, or the selectFromResult function itself changes - it should only re-execute selectFromResult in one of those two cases.

[kkirby]

Yes, however, selectFromResult doesn't need to be invoked again on every render. If the function itself has not changed, there is no reason to think the return value of it would have changed either. My thought is that the result of the selector function should be cached and should only be invalidated if a new function is provided (or if the data being sent to the selector has changed).

In my example in my first post, the function I'm passing to selectFromResult will only change if postID changes.

Hopefully that makes sense?

[phryneas]

It makes sense, and it is exactly what the code that I linked to should do - but I see in your reproduction that it doesn't, which is deeply confusing.

[phryneas]

Although the way we call that might be at fault here, with querySelector(state, lastValue.current), where lastValue.current could change a tad more often. 🤔

Maybe we need this additional check?

diff --git a/packages/toolkit/src/query/react/buildHooks.ts b/packages/toolkit/src/query/react/buildHooks.ts
index ba86d843..fbff7d24 100644
--- a/packages/toolkit/src/query/react/buildHooks.ts
+++ b/packages/toolkit/src/query/react/buildHooks.ts
@@ -918,7 +918,12 @@ export function buildHooks<Definitions extends EndpointDefinitions>({
               (_: ApiRootState, lastResult: any) => lastResult,
               (_: ApiRootState) => stableArg,
             ],
-            queryStatePreSelector
+            queryStatePreSelector,
+            {
+              memoizeOptions: {
+                resultEqualityCheck: shallowEqual
+              }
+            }
           ),
         [select, stableArg]

[kkirby]

I see what you mean now. I wasn't really aware of how createSelector worked, but you're right, the expectation is that the function should not be called in this case.

There is a lot going on at play in buildHooks, and I've just started learning about RTK Query, however I think your proposed solution might be correct? The object being passed into selectFromResult is different for each render. However, doing a simple stringify of the object shows that the actual data has not changed. So, I think that you're correct: createSelector is seeing that the object passed in has changed, even though the properties of it have not.

-- addendum --

If that is the case (and I'm understanding createSelector correctly), you'd probably see a significant speed increase in rendering :P.

[phryneas]

This will be fixed with #4029 - thanks to @thisjeremiah for the PR! :)