reduxjs / redux-toolkit

The official, opinionated, batteries-included toolset for efficient Redux development
https://redux-toolkit.js.org
MIT License
10.74k stars 1.18k forks source link

RTK Query returns cached data for different query arguments, unless remount is forced #4575

Closed thomasmarr closed 1 month ago

thomasmarr commented 2 months ago

I do not think this is expected behavior, but could be mistaken. Any advice would be appreciated.

I have query definition like this:

profilePhoto: builder.query<string, string | undefined>({
  query: (entraid_user_id: string) => ({
    url: `users/${entraid_user_id}/photos/240x240/$value`,
    responseHandler: (response) => {
      if (response.ok) return response.blob();
      throw response;
    },
  }),
  transformResponse: (response: Blob) => {
    return URL.createObjectURL(response);
  },
  providesTags: (_, __, entraid_user_id) => [
    { type: "profilePhoto", id: entraid_user_id },
  ],
}),

It's called in a component like this:

const ProfilePhoto = ({ user }: { user: User }) => {
  const { data, isLoading } = useProfilePhotoQuery(user.entraid_user_id);
  // ...

When that component re-renders with a new user prop, (i.e. with a different value for user.entraid_user_id), I can see the correct network request firing, but while that request is in flight the value of data returned by the query hook is the value for the previously rendered user, and the value of isLoading remains false.

When the network request is rejected (404), still the value of data is for the previously rendered user, and isLoading remains false.

So both while the network request is in flight and after it resolves, the other rendered profile details match the newly rendered user, but the profile photo is of the wrong person.

If I set the query option { refetchOnMountOrArgChange: true } this has no effect. Same behavior.

If I force a remount by giving the component a key={user.id} prop, then it behaves as expected. data is undefined, isLoading is true, and when the 404 is returned, data remains undefined and isLoading is false. In this case it doesn't matter whether I set refetchOnMountOrArgChange - it works with or without it.

I have lots of experience with tanstack query, but thiis my first time using RTKQuery. Is this expected behavior? Seems a bit counterintuitive to me if it is.

Thanks.

markerikson commented 2 months ago

Those are both expected, yes:

For more details, see:

thomasmarr commented 2 months ago

Ok, thanks for taking the time to reply (especially given it's explained in the docs - I swear I looked before posting the issue).

It still feels counterintuitive to me, as though the default behavior should be the other way around. I would have thought the most common use case for a query hook with args is to return a different resource based on the provided arg.

// first render
const { data, isLoading } = useGetPokemonQuery(pikachu.id) // <- data is pikachu

// second render
const { data, isLoading } = useGetPokemonQuery(bulbasaur.id) // <- data is still pikachu? this is unexpected

Anyway I'm nitpicking. This issue can be closed. Thanks again.

phryneas commented 2 months ago

The idea here is to prevent big layout changes. Usually it's preferrable to keep old data on the screen for a second longer instead of having the whole screen replaced with a spinner and then immediately have the data flicker in 50ms later. It's also what React Suspense does (read: will do when it's finally officially supported on the client) - old data stays on new screen until new data is fetched.

rjgotten commented 2 months ago

@phryneas It's also what React Suspense does (read: will do when it's finally officially supported on the client) - old data stays on new screen until new data is fetched.

No. It's not. <Suspense> renders the fallback content should any component within it suspend. If you want to keep showing existing content you need to actually do work for that and need to arrange code to use the useDeferredValue hook or leverage startTransition. The default is to show the fallback content - i.e. 'the spinner' in this story.

maksnester commented 2 weeks ago

I understand why it is done this way (to embrace showing the cached data while fetching things in background) but also it causes A LOT of headache sometimes. For example I have to wrap some hooks with code like this:

export const useGetNodesQuery = (itemId?: string): CurrentItemReturnType => {
  const { data: currentItem, isLoading: isLoadingCurrentItem } =
    useGetItemQuery(itemId!, {
      skip: !itemId,
    });

  // when query is dispatched with a new argument, the query hook returns previous data,
  // and it also not marked as isLoading if there is any previous data
  // this is done to embrace showing cached data
  // but for this specific case we do not want to use the "previous" data as it is irrelevant
  if (currentItem && currentItem.id !== Number(itemId)) {
    return {
      currentItem: undefined,
      isLoadingCurrentItem: true,
      currentItemId: itemId!,
    };
  }

  return {
    currentItem,
    isLoadingCurrentItem,
    currentItemId: itemId!,
  };
};

I do not want to refetchOnMount because, well, I want to keep the cached data, I just do not want the stale data to be available when it is completely irrelevant for me.

Having some flag to accommodate these cases would be helpful. I'd expect RTKQ to be able to simply treat arg change as working with a new query, i.e. trigger the isLoading state, give data as undefined.

@markerikson can it be reconsidered? Or maybe there is something already that handles this use case?

EskiMojo14 commented 2 weeks ago

@maksnester have you checked out currentData, mentioned in Mark's comment? it'll always match the current arg, or be undefined if it's still fetching.

You also might want isFetching rather than isLoading.

markerikson commented 2 weeks ago

Yeah. isLoading vs isFetching is admittedly something people get confused over, but most of the time you really want isFetching:

EskiMojo14 commented 2 weeks ago

the way i like to think of it is:

maksnester commented 2 weeks ago

@EskiMojo14 @markerikson thank you guys, I skimmed through the topic a bit too quickly and didn't notice the currentData (didn't know about it).