rtk-incubator / rtk-query

Data fetching and caching addon for Redux Toolkit
https://redux-toolkit.js.org/rtk-query/overview
MIT License
626 stars 31 forks source link

TypeError getting `isFetching` from `use*Query` when `selectFromResult` is used. #219

Closed StefanBRas closed 3 years ago

StefanBRas commented 3 years ago

I wanted to use selectFromResult but found that to have isSuccess and the likes, they would also have to be passed through this selector.

However this does not work for isFetching, and gives the error:

Property 'isFetching' does not exist on type '({ status: QueryStatus.uninitialized; originalArgs?: undefined; data?: undefined; error?: undefined; requestId?: undefined; endpointName?: string | undefined; startedTimeStamp?: undefined; fulfilledTimeStamp?: undefined; } & { ...; }) | ({ ...; } & { ...; }) | ({ ...; } & { ...; }) | ({ ...; } & { ...; })'.ts(2339)

Am i missing something with how this is to be used, or is it a bug?

For example this will work:

const { data, isSuccess } = useGetPokemonByNameQuery(name, {
    selectFromResult: ({ data, isSuccess }) => ({ data, isSuccess })
  });

This will give a a type error:

const { data, isFetching } = useGetPokemonByNameQuery(name, {
    selectFromResult: ({ data, isFetching }) => ({ data, isFetching })
  });

Here's a codesandbox to show it: https://codesandbox.io/s/concepts-queries-deduping-caching-forked-b4u4z?file=/src/Pokemon.tsx

phryneas commented 3 years ago

isFetching is actually not a part of currentState/result (which at that point really just comes from redux state) and needs to be calculated on a per-hook-level (because it needs information about the state of the last render of this component), by calling defaultQueryStateSelector, passed in as third argument into that selector function.

So, correct code would be

  const {
    data: selectFromResultWithIsFetching,
    isFetching
  } = useGetPokemonByNameQuery(name, {
    selectFromResult: (result, lastResult: {data: any, isFetching: boolean} | undefined, defaultSelector) => {
      const { data, isFetching } = defaultSelector(result, {...lastResult});
      return { data, isFetching }
    }
  });

We clearly need to document this.

On that note: the isLoading you extracted there might also not be the same isLoading that you would get from the hook itself, as that, too depends on the values of the last render. I get that this might be confusing, but atm. I'm not too sure what to best do about it :thinking: (see the implementation of defaultQueryStateSelector: https://github.com/rtk-incubator/rtk-query/blob/c769479a056f4ddf0972ae9bff19a2ca4352751f/src/react-hooks/buildHooks.ts#L287-L299 )

Also, this uncovered a bug in the types - this should also be possible to do as just

  const {
    data: selectFromResultWithIsFetching,
    isFetching
  } = useGetPokemonByNameQuery(name, {
-    selectFromResult: (result, lastResult: {data: any, isFetching: boolean} | undefined, defaultSelector) => {
+    selectFromResult: (result, lastResult, defaultSelector) => {
-      const { data, isFetching } = defaultSelector(result, {...lastResult});
+      const { data, isFetching } = defaultSelector(result, lastResult);
      return { data, isFetching }
    }
  });

which it currently isn't. So we probably need changes like

export type QueryStateSelector<R, D extends QueryDefinition<any, any, any, any>> = (
  state: QueryResultSelectorResult<D>,
-  lastResult: R | undefined,
+  lastResult: NoInfer<R> | undefined,
  defaultQueryStateSelector: DefaultQueryStateSelector<D>
) => R;

export type DefaultQueryStateSelector<D extends QueryDefinition<any, any, any, any>> = (
  state: QueryResultSelectorResult<D>,
-  lastResult: Pick<UseQueryStateDefaultResult<D>, 'data'>
+  lastResult: Pick<UseQueryStateDefaultResult<D>, 'data'> | undefined
) => UseQueryStateDefaultResult<D>;

for this to be used better in the next version.

Also, we should consider generally simplifying UseQueryStateDefaultResult for readability, even if it means some code duplication.

StefanBRas commented 3 years ago

Thanks for the throughout answer!

Regarding documentation, I at first only looked at the selecing data from a query result section, which made it seem like selectFromResult just takes the data and filters it. Now from looking at the useQuery api reference i see that selectFromResult takes a QueryStateSelector which isn't mentioned anywhere in the docs.

Hope this isn't coming of as complaning - I just wanted to add how I as a new user of RTK-query read the documentation.

And just to confirm - in the case where I want to select a subset of a query and get IsFetching in a component, your first code block would be the correct way to do it?

phryneas commented 3 years ago

Yeah, there is still a lot of stuff to be documented and some edges are also only found by early adopters such as you :)

And just to confirm - in the case where I want to select a subset of a query and get IsFetching in a component, your first code block would be the correct way to do it?

Yes (the same for isLoading) with one caveat: if your subset would be undefined at times, while result.data would be present, actually both isLoading and isFetching might both return to true where it would not be if you returned result.data.

Might need to think more about that. Maybe we can do that better in the next release.

phryneas commented 3 years ago

This should get handled better after we get https://github.com/reduxjs/redux-toolkit/pull/1035 in

phryneas commented 3 years ago

Closing this here.