RFC: remove callbacks from useQuery

[TkDodo]

I wanted to write an RFC, but after I announced it on twitter, I decided to make a blogpost. This should cover most of the questions around that topic:

https://tkdodo.eu/blog/breaking-react-querys-api-on-purpose

[mikechabot]

+1 for this change, and great article. It always seemed like an unnecessary footgun that I try to catch in PRs.

For onSuccess, I've seen multiple use cases, but the below seem the most common:

1. UI notifications
2. Data manipulation (massage the response further and manually call setQueryData)
3. Chain calls (response from A is required for request B)

Problem with these approaches:

1. For useQuery, I've yet to see a useful case for notifying end-users of a successful /GET or GQL /POST. I'm sure they're out there, but overall I'd still question the usefulness of it.
2. Data manipulation should be performed lower in the stack, closer to the pure fetch, or nearer to the component that requires further data massaging. More on this in a second.
3. Chain calls: either follow the dependent-queries pattern, chain calls lower in the stack, or lastly (and grossly), leave it to the React lifecycle and cook up some homegrown useEffect chain that you'll hate debugging later.

As for UI notifications, regardless of whether its useEffect or useMutation, I don't like crossing the boundaries set in the service pattern described below. Call me old fashioned, but SOC comes to mind: mixing UI state with API state doesn't feel good. Sure, you save yourself a useEffect, but I'd argue that co-locating notifications with API state-caching isn't the cleanest breakdown.

For what it's worth, here are some guidelines I've set down for myself and my team. These are not hard-and-fast rules, but rather a mental model of what should be called where, and who should be doing what.

---

Service Layer Pattern

Layer 1: Pure network call

• This layer handles nothing more than executing a network call and returning data to the caller.
• It may perform additional logic on the HTTP response, such as mapping a complex object to something more consumable for the front-end, or normalizing errors:

export const fetchById= (id: string) => {
  try {
    const data = await apiClient.getById(id);
    return mapBadAPIContractToGoodClientContract(data)
  } catch (e: unknown) {
    throw ServiceErrorFactory.create(e)
  }
};

Layer 2: API State (Hook)

• This layer handles nothing more than executing a Layer 1 network call, and caching it in react-query.
• Pass in options, change stale times, do whatever react-query allows, but please, please don't rely on onSuccess / onError.
  • If you need to manipulate data for all callers, do it in Layer 1.
  • If you need to manipulate data for specific callers, do it in Layer 3.
  • If you need to always chain calls for a meaningful response, do it in Layer 1, otherwise, allow react-query to cache each separately, and follow dependent-queries.
  • If you need to show notifications, do it in Layer 3 or Layer 4.

export const useFetchById= (id: string) => {
 const cacheKey = [CacheKey.FetchById, id]

  return useQuery<FetchByIdResponse, ServiceError>(
    cacheKey,
    () => fetchById(id), 
    {
      enabled: Boolean(id)
    },
  )
}

Layer 3: Page Hook (Optional)

• Optionally wrap the Layer 2 hook to reduce business logic within React components, and to remove it fully from Layer 2.

export const usePageWithId= (id: string) => {
  const { 
    data, 
    error, 
    isLoading, 
    isSuccess, 
    isError 
} = useFetchById(id)

  /**
   * Optionally map the network error to a React component
   */
  let errorFallbackComponent: ReactNode | undefined
  if (isError) {
    errorFallbackComponent = mapErrorToFallbackComponent(error?.code)
  }

  /**
   * Optionally map response data to Component page props
   */
  let props: PageWithIdProps| undefined
  if (data) {
    props = mapMyDataToMyComponentProps(data)
  }

  /**
   * Optionally take action in the event of a successful call
   */
  useEffect(() => {
    if (isSucces) {
      showToastNotification('Success!') 
    }
  }, [isSuccess])

  /**
   * Optionally take action in the event of a failed call
   */
  useEffect(() => {
    if (isError) {
      showToastNotification('Error!') 
    }
  }, [isError])

  return { props, isLoading, isError, errorFallbackComponent }
}

Layer 4: Page Building

• Either use your page hook from Layer 3, or just call your useQuery hook from Layer 2:

export const PageWithId= ({id}: {id: string}) => {
  const {
    props,
    isError,
    isLoading,  
    errorFallbackComponent,
  } = usePageWithId(id);

// Don't have Layer 3? Just call your useQuery hook normally
//  const { 
//    data, 
//    error, 
//    isLoading, 
//    isSuccess, 
//    isError 
//  } = useFetchById(id)

  if (isLoading) {
    return <Loading />;
  }

  if (isError) {
    return errorFallbackComponent;
  }

  return <PageTemplate {...props} />;
};

Okay, this turned out longer than I anticipated, but I just wanted to provide a scalable pattern in the event this change kicks up more dust.

[MrSharpp]

You are correct! The UI-related things should be done in the business logic layer, hence notifications/toast/alerts should be written in layers 3-4.
Thats really neat, thanks!

[maertu]

I currently use onSuccess to breaking down a list. f.e. I have an endpoint giving me all restaurants. and onSuccess i call
response.forEach(restaurant => setQueryData(["restaurants", "restaurant.id"], restaurant)
Do i understand you correct that it would be better to pass queryClient to the api-fetching method (layer1) and call setQueryData in there, after i received the list of restaurants?

[TkDodo]

i have a blogpost on this: https://tkdodo.eu/blog/seeding-the-query-cache

[hrachhovsepyanqless]

What about "useQueries"? If the callbacks are removed, is it even possible to write a useEffect that reacts to the fetched data change? I have a specific case where the number of queries is dynamic and I perform some calculations based on the index of the query, that was successful.

[TkDodo]

for calculations, you don't need onSuccess or useEffect. Just derive the state in the render function:

const queries = useQueries(...)

const derived = deriveStateFromQueries(queries)

for v5, we're working on a new combine API:

• feat(useQueries): combine  #5219

[omermizr]

In the blogpost you mentioned onDataChanged for state-syncing purposes and said the solution should be a useEffect in userland. That means that whenever the data changes, for every mounted instance of that query that effect will run, right?
That doesn't sound like something I'd want TBH. Multiple calls to sync (dispatch in Redux), performance etc.
WDYT?

BTW I don't use these callbacks for queries in my app, only in mutations, but I can see the use-case.
Love query, keep up the good work!

[TkDodo]

That means that whenever the data changes, for every mounted instance of that query that effect will run, right?
That doesn't sound like something I'd want TBH. Multiple calls to sync (dispatch in Redux), performance etc.
WDYT?

yes, that is exactly what is now happening with onSuccess as well. With every successful fetch, every mounted instance will dispatch to redux.

with the custom effect, it would be a little bit better, because it wouldn't necessarily happen on every fetch: if what is returned from your queryFn is the same as before, the effect won't run again (and react-query has structural sharing, so it will really be deep equal), whereas onSuccess always runs when a fetch was successful.

I generally think one shouldn't be doing state syncing at all, it's an anti-pattern, and if you do it, you now have to write a useEffect and maybe that alone will discourage some users :)

[omermizr]

I didn't remember that onSuccess runs for every mounted instance! I guess that's why I chose not to use it in the first place 🤷‍♂️

[Wyatt-SG]

My team currently uses onSuccess for poll job queries like the following:

https://gist.github.com/Wyatt-SG/2d7f93f50956954857bd8e6b3f447275

With this change what would be your recommended pattern?

[jasikpark]

👀

[priyajeet]

@TkDodo what would now be a clean/recommended way to achieve something like below. Example below essentially is a poll via refetchInterval but only if the response data.hasSomething otherwise poll/try n-times and stop.

function usePoll({ interval, limit }) {
  const count = React.useRef(0);

  function onSuccess(data) {
    if (data.hasSomething) {
      count.current = 0;
    } else {
      count.current += 1;
    }
  }

  function refetchInterval(query) {
    if (count.current >= limit || query.state.error) {
      return false;
    }

    return interval;
  }

  return useQuery({ onSuccess, refetchInterval, ... });
}

Do we now have to delegate that to the QueryCache's onXXX global callbacks and pass in references via meta? Or does one now instead need utilize some of the stuff available on QueryState? I see that QueryState has dataUpdateCount, but doesn't seem to be returned in the useQuery result like the failureCount, unless I am blind. Didn't see docs around it. So roughly something along the lines of:

function usePoll({ interval, limit }) {
  const dataUpdatedCount = React.useRef(0);
  const retryCount = React.useRef(0);

  function refetchInterval(query) {
    if (dataUpdatedCount.current < query.state.dataUpdatedCount) {
        dataUpdatedCount.current = query.state.dataUpdatedCount;

        if (query.state.data.hasSomething) {
          retryCount.current = 0;
        } else {
          retryCount.current += 1;
        }
    }

    if (retryCount.current >= limit || query.state.error) {
      return false;
    }

    return interval;
  }

  return useQuery({ onSuccess, refetchInterval, ... });
}

[TkDodo]

I think this looks good 👍

[priyajeet]

Cool, a followup question: Any reason why the dataUpdatedCount is not returned from useQuery()? Can it be also returned similar to how errorUpdatedCount and fetchFailureCount is returned? dataUpdatedAt is also returned.

Other utility numbers that can help is maybe successCount that resets when failure happens, similar to how failureCount resets when success happens.

BTW errorUpdatedCount is not documented here but is documented in the description section on that page.

[Jokero]

Yeah, it seems dataUpdateCount is missing by some reason. It would be nice to have it for consistency. @TkDodo, do you maybe know why it's not added to query result?

[MrSharpp]

Instead of removing callbacks from useQuery.

Can't we just compare the queryFn returned data and if data are different then call onSucess?

Like adding this where ever the callback is calling

[TkDodo]

what is authCallback doing? why is it a query ?

[Raj-G07]

export const appRouter = router({

authCallback: publicProcedure.query(async () => {
const { getUser } = getKindeServerSession()
const user =await getUser()

if (!user?.id || !user?.email)
  throw new TRPCError({ code: 'UNAUTHORIZED' })
  return {success:true}

}),
})

[TkDodo]

Then redirect in the queryFn

[Raj-G07]

Can you explain it briefly ?

[0xtz]

having the same case with @Raj-G07 hhh

I have a trpc query and I need to toast after it finishes the fetching hhhhh

[ShravanSunder]

@TkDodo So currently we use the callbacks to update query parameters after making sure that it won't create a rendering loop.

/**
 * Query repeating template from the API
 */
export const useLoadeQuery = (
  paginationQuery: Partial<IPagination>
): QueryObserverResult<IRepeatingTemplateTableResponse> => {
  const { searchQuery } = useTableSearch();
  const { filterQuery } = useTableFilter();

  const client = useQueryClient();
  const { updateSearchParams } =
    useNavigationSearchParams<Partial<IPagination>>();

  const apiResult = useQuery(
    [
      "useLoadRepeatingTemplateQuery",
      searchQuery,
      paginationQuery.orderBy,
      paginationQuery.orderDirection,
      paginationQuery.pageNumber,
      paginationQuery.pageSize,
      filterQuery,
    ],
    queryApi(() => {
      return apiRequests().getRepeatingTemplateListAsync(
        searchQuery,
        paginationQuery,
        filterQuery
      );
    }),
    {
      notifyOnChangeProps: ["data", "isLoading"],
      refetchOnMount: "always",
      onSuccess: (result: IRepeatingTemplateTableResponse) => {
        const isSame =
          paginationQuery.orderBy === result.pagination.orderBy &&
          paginationQuery.orderDirection === result.pagination.orderDirection &&
          paginationQuery.pageNumber === result.pagination.pageNumber &&
          paginationQuery.pageSize === result.pagination.pageSize;

        if (!isSame) {
          // the array below should be the same order as above cache key used in react query
          client.setQueryData(
            [
              "useLoadRepeatingTemplateQuery",
              searchQuery,
              result.pagination.orderBy,
              result.pagination.orderDirection,
              result.pagination.pageNumber,
              result.pagination.pageSize,
              filterQuery.searchWithin,
              filterQuery.startDate,
              filterQuery.endDate,
            ],
            result
          );
          //update the url params with the result pagination from the back end to make sure the data and url parms are in sync
          updateSearchParams(result.pagination);
        }
      },
    }
  );
  return apiResult;
};

We also use it to update our redux store when certain data is required by client side state or thunks

For example organisation data that we need in redux state

/**
 * Calls the organization info API
 * Should use hook directly in react components or in things in the react lifecycle.
 * Access to org info in redux store should only be within thunk calls. If not use this hook!
 * @returns
 */
export const useOrganisationInfoQuery =
  (): QueryObserverResult<IOrganisationInfo> => {
    const dispatch = useDispatch();

    const apiResult = useQuery(
      ["getOrganizationInfoAsync"],
      queryApi(apiRequests().getOrganisationInfoAsync),
      {
        onSuccess: (result: IOrganisationInfo) => {
          dispatch(commonActions.setOrganisationInfo(result));
        },
        onError: () => {
          dispatch(commonActions.setOrganisationInfo(undefined));
        },
      }
    );

    return apiResult;
  };

Do you think there are better ways to do these? The useEffect loop ads more render cycles and dependency gotcha's to this whole process. How else would you recommend to do this?

[TkDodo]

I'd recommend not duplicating state. The write to redux seems unnecessary - why do you need data from useQuery in redux just to read it with useSelector, when instead, you could just read it with useQuery again?

The url params is a similar story. Ideally, the url params are the single source of truth for your input so you don't need to update it from the output. Not sure what result.pagination contains, but it seems like you have that information available as input in paginationQuery ?

[TkDodo]

IF you need to do things like that then yeah, useEffect it is. As stated in the blog post, it's a good thing that you kind of don't want that, so you get the incentive to refactor it to a different approach maybe :)

[ShravanSunder]

@TkDodo hmm, we have a app where we were saving server data into redux (and using the old thunks pattern). Currently we are moving away from this. We wanted to consolidate the organisation data fetchs so that everything (sever data) goes through react-query and instead of using the old thunk pattern, save onsuccess to redux so the rest of the app could access organisation data from the redux state.

Sounds like this pattern should be using useEffect instead now?

[TkDodo]

save onsuccess to redux so the rest of the app could access organisation data from the redux state.

If you must do that, useEffect is better, yes.

However, you can also consider that the rest of the app can still access data by combining it with redux selectors:

const useSomething = () => {
  const { data } = useMyQuery(...)
  return useSelector(state => combineFromStateAndData(state, data))
}

it really just comes down to how much effort that is in your app.

[matthewwolfe]

This is only for useQuery correct? useMutation will continue to have the callback functions?

[DamianOsipiuk]

Yes

[scottrepreneur]

Can you clarify why the pattern is acceptable for useMutation?

[TkDodo]

because mutations are triggered imperatively and the common use-case is to invalidate after a mutation. It's also necessary for optimistic updates. Generally, queries and mutations work differently under the hood.

[JoostKersjes]

I'm probably a bit late to the party, but I still wanted to contribute. For some background, the app I'm working on is a Vue3 SPA learning platform that has customized content for each "environment" (usually one per company). We use @tanstack/vue-query to interface with the API for our backend.

Usage

We have a handful of queries that use these callbacks, so not wide-spread usage, which is good news for when we need to migrate.

Here is a list of all the things that currently happen in our useQuery callbacks:

onSuccess

1. Profile --> Send user ID to analytics (if enabled)
2. Preferences --> Use locale to set/change i18n global locale, saved to localStorage
3. Environment --> Use theme name to apply styles for theme, saved to localStorage
4. Environment --> Set document.title using environment name e.g. Home | [name], saved to localStorage
5. Environment --> Activate analytics plugin if analytics are enabled in environment settings
6. Environment --> If the environment only has one available locale, use it to set/change i18n global locale

Some things are saved to localStorage so the next time the user visits the app, we can use the stored values for the initial render so users don't see a flash render of a different theme/language. The onSuccess callback is the only place we write to localStorage.

onSettled

1. Onboarding --> If there was an error fetching the onboarding state or some parts are not completed, we show a tutorial

Looking at the list above, I can see there are certainly some odd choices that we made. However I feel that most of these side effects are easiest to maintain if the query result is responsible for triggering them.

Moving e.g. the analytics side effect to a component just doesn't make sense as it is something that affects the entire app.

Quick note on errors: We do error handling in the components that use our queries because that makes more sense in our designs.

Moving forward

As an example, this is how I would replace the onSuccess for the preferences:

// before
export const usePreferencesQuery = () => {
  return useQuery({
    // queryKey, queryFn, retry
    onSuccess: (data) => {
      applyLocale(data.locale);
    },
  });
};

// after
export const usePreferencesQuery = () => {
  const query = useQuery({
    // queryKey, queryFn, retry
  });

  watch(query.data, (data) => {
    if (!data) return;
    // query success
    applyLocale(data.locale);
  });

  return query;
};

The logic flow difference is minor, but just to mention it, the data parameter is now potentially undefined, so an early return is needed to match onSuccess.

The bigger difference to me is that I now feel the need to clarify that the code only runs if the query was successful. It isn't obvious any more from the code itself. I feel tempted to create a set of utility functions that mirror the old callback functions simply so the intentions of the code are easier to read.

// I'm extremely tempted to make this a util
// Please convince me that I shouldn't
export const onQuerySuccess = <TData, TError>(
  query: UseQueryReturnType<TData, TError>,
  cb: (data: TData) => void
) => {
  return watch(query.data, (data) => {
    if (!data) return;
    cb(data);
  });
};

export const usePreferencesQuery = () => {
  const query = useQuery({
    // queryKey, queryFn, retry
  });

  // This is far nicer to read, clearer business logic
  onQuerySuccess(query, (data) => {
    applyLocale(data.locale);
  });

  return query;
};

Responding to blog

I appreciate that React is the most popular framework, I can still read the code, but my work has been in the Vue bubble for over a year now. Each framework deals with render cycles and reactivity differently, so the implications for rendering might not be as bad or even non-existent.

I think that the i18n locale state is a good point of discussion. Technically speaking it is "state syncing", but I don't (yet) see a better place to update the user's locale than after the API request for their preferences has succeeded.

As closing thoughts, I think that removing these callbacks from the useQuery API is probably a big improvement for the large majority of use cases. It also simplifies the API even further, which I always appreciate! I'm still left with the question if the side effects in my app need to be improved in some way, since the pattern is being discouraged.

Any and all feedback is greatly appreciated!

[TkDodo]

No matter the approach, with a plugin like vue-i18n that keeps it own state, there will always need to be some amount of "state syncing".

👍

I assumed persisters could only save the entire state of a queryClient, but I'll go take a proper look. Thanks again!

yes but you can filter which queries you'd want to persist

[teobmg]

IMO this is a really good example to keep the callbacks.

Moving everything inside the queryFn sounds like a coupling that can be easily avoided thanks to callbacks.

[TkDodo]

IMO this is a really good example to keep the callbacks.

it's really not. Did you read the article where I point out that callbacks, as they currently are, are either called too often (once per observer) or not often enough / not at all when you have staleTime set ...

[teobmg]

as they currently are
To me, this is the heart of the discussion.

Their current implementation could bring some bugs if you don't know the internals, that's true, but the solution is not to entirely remove the API and replace it with the useEffect. The solution should be: clearly document its behavior and also try to change it in order to enhance DX.

Additionally, reading one of your comments in this thread, you say that the callbacks remain for the mutations

because mutations are triggered imperatively

Even queries can be triggered imperatively, using the enabled parameter, so they could have callbacks as well.

I mean...we are creating a discrepancy between the APIs that will make the dev's life harder.

[TkDodo]

Even queries can be triggered imperatively, using the enabled parameter, so they could have callbacks as well.

it's still declarative, just conditional / deferred.

The solution should be: clearly document its behavior and also try to change it in order to enhance DX.

how should we change it please? If you only run it once per query, you can easily get into stale closure issues, because it's not clear for which observer it will run (it would be a random one). And if we only read data from the cache, we cannot invoke a side-effectful callback during render, so we need to spawn an effect internally. If you want that, make an effect in user-land 🤷

[jasikpark]

It would be awesome to have a lint rule or something so that it's possible to disable these callbacks on a v4 version before upgrading to v5, since this seems like one of the more complicated changes requires other than simple API changes

[jasikpark]

I ended up patching @tanstack/query-core/build/lib/types.d.ts b/build/lib/types.d.ts with pnpm patch to remove the callbacks from v4.29.7 since there's no other way to warn on using a deprecated option :p

ref: https://discord.com/channels/719702312431386674/1108114206252875776

[TkDodo]

most people I talked to don't even know those callbacks exist 😅

[jasikpark]

🤷 had several places where my project was using them, for example to store a previous response under a "prevCursor" cache key

[liect]

[TkDodo]

if you need to execute side effects after a query occurs, this "unnecessary template code" is at least explicit. I personally have a hard time finding a use-case for a side effect that has to run every time after a query errors that cannot be handled by the global cache callbacks. Do you have a concrete example?

[Arsenicro]

I have a case where I have to use some hooks in my onError function (like translations, toast, logout on 401 etc.). My current solution is still far from ideal, but I'd like to know if I'm missing something simple. Currently, to every query, I'm passing an onError function from my custom useError hooks, which can use all necessary hooks.
As I understand, with those changes, I need to change it, and your suggestion is to move it to QueryClient. While it is a good idea to move logic to one place, I still need to somehow solve an issue with using hooks in onError function, and because I also need to use QueryClinet as an object, it is not as simple. For now, I have in mind a solution that moves the creation of QueryClient to the custom Context/Hook or passes all dependencies in meta tag; both seems far from ideal.
Let's say that the simplified example looks something like that:

export function useTodos() {
  const { t } = useTranslation();
  const { setNotification } = useNotifications();

  return useQuery({
    queryKey: ['todos', 'list'],
    queryFn: fetchTodos,
    onError: () => {
      setNotification('error', t('errors.error'))
    },
  })
}

Can you suggest some solution to this issue?

[TkDodo]

function App() {
  const { t } = useTranslation();
  const { setNotification } = useNotifications();

  const [queryClient] = React.useState(() => new QueryClient({
    queryCache: new QueryCache({
      onError: (error) => setNotification('error', t('errors.error'))
    })
  }))
}

Note that this will close over t during the useState initializer, so if you expect translations to change, this will not work.

[Arsenicro]

So what with the case when I'm expecting translations to change? Or setNotification to change? Should I use useEffect?

[TkDodo]

yes you can spawn a useEffect that updates queryCache.config.onError

[Karbust]

What would be the correct way to handle the errors?

I have a constructQuery method so I don't have to declare everything again and again on every page, just need to pass some arguments, then I have: onError: (error) => enqueueSnackbar(errorMessage, { variant: 'error' }),

I'm currently only using useQuery, then pass the constructQuery method inside.

[TkDodo]

did you read the blogpost? That's what the global cache callbacks are for ...

[fobos531]

@TkDodo Which approach exactly do you recommend if I have a tad more logic in my onError than just simple notifications/toasts? For example, I have a use case where if the API call fails and the query errors, the logical thing to do is to set a a form field's state (using react-hook-form) to a very specific value. What approach do you recommend in this case?

[TkDodo]

hard to say without fully knowing what this is about. Could be separating the query and the form into two components. If everything fails, you can always useEffect.

[RobinVdBroeck]

I found another bug in some code of mine that could have been prevented by this, so I agree that this is a footgun and should be removed.

function usePagination(key, page) {
   const queryClient = useQueryClient();
   return useQuery({
      key,
      queryFn: doCall(page),
      onSuccess() {
        queryClient.prefetch(nextPageKey, () => doCall(page + 1))
      },
   });
}

When I increased the stale time on this prefetching stoped to work for every other page, since the next page queries where not stale anymore, they did not to the request anymore, not triggering the onSuccess anymore.

[VesperQuartz]

So how do you do your pre-fetching now?

[TkDodo]

• in an event handler (user interaction)
• in an effect

[J3tto]

I have a case where I have two useQuery hooks, one is connected to a endpoint which returns a list, the other an endpoint which returns a single object. In both cases each item returned also contains a version property which I use to understand if any object has chanaged.

Currently I use the onSuccess callbacks in each useQuery to update the queryData of the other key, but only when the version has changed. In light of the planned removal of onSuccess how would be best to migrate these?

Simplified example:

export const useUsers = () => {
  const queryClient = useQueryClient();
  return useQuery<User[], Error>({
    queryKey: ['user','list'],
    queryFn: async () => getUsers(),
    onSuccess: (data) => {
      data.forEach((user) => {
        const cacheKey = ['user', { id: user.id }];
	const oldCache = queryClient.getQueryData<User>(cacheKey);
        if (!oldCache || user.version > oldCache.version) {
	  console.debug('Updated cache for', cacheKey);
          queryClient.setQueryData(cacheKey, user);
      });
    }
  });
};


export const useUser = (userId: string | undefined) => {
  const queryClient = useQueryClient();
  return useQuery<User, Error>({
    queryKey: ['user', { id: userId }],
    queryFn: async () => getUser(userId),
    enabled: Boolean(userId),
    onSuccess: (user) => {
      const cacheKey = ['user','list'];
      const oldCache = queryClient.getQueryData<User[]>(cacheKey);
      if (!oldCache || user.version > oldCache.version) {
        console.debug('Updated cache for', ...cacheKey);
        queryClient.setQueryData(cacheKey, (old: User[] = []) => [...old.filter((u) => u.id !== user.id), user]);
      }
    }
  });
};

[TkDodo]

https://tkdodo.eu/blog/seeding-the-query-cache#seeding-details-from-lists

[J3tto]

Ahh perfect! many thanks!

I see i need to move the logic into the queryFn.

[AshotN]

One thing I use onSuccess for is caching a FIND request by its individual items.

Take this example,
https://github.com/ampache/ampache/blob/dabd2e1d2c2dc5cd71c852b3afa6e5b8fcb37e30/client/logic/Playlist.ts#L38

If I am getting all the playlists from the server, I may as well cache the individual ones to avoid having to pull them later. How would I go about doing this in the API?

[TkDodo]

it's literally this:

• RFC: remove callbacks from useQuery #5279 (reply in thread)

[AshotN]

Oh I had missed that. That's smarter than what I was doing. Thanks!

[ChambreNoire]

I currently use onSuccess passed to a wrapped useQuery in order to update an Antd form upon a specific error.

My context is an entity creation/update form (let's say a Project entity) displayed in a dialog box. In the rare cases when there is a concurrency conflict, a popup is display informing the user of the conflict and inviting them to reload the entity and reset the edition form using the new info. The Dialog has useProject and useUpdateProject hooks which wrap useQuery and useMutation respectively. useUpdateProject handles the error in onError and displays the popup. When the 'reload' button is clicked it invalidates the cache pertaining to the specific Project instance in order to reload it from the server. The Antd form then needs to be repopulated and so I pass an onSuccess to the useProject hook in which this is done.

It's not great and the removal of callbacks from useQuery will scupper this approach.

Any ideas?

Thanks!

[ChambreNoire]

Except the form values will have been changed by the user before this. If I set the form values in the useEffect, the form will only revert to the server state if there has been a change to the server-side entity. Using onSuccess had the advantage of the callback being triggered upon invalidation regardless of whether the server-side data had changed.

[fitterfizzle]

I totally agree with this, query invalidation used to reset the form but with a useEffect with only query.data this doesn't happen anymore. How should we tackle this case? We have a huge code base that relies exactly on this pattern...any suggestions @TkDodo ?

[TkDodo]

if you want to reset the form, reset the form. Why would you rely on invalidation to do this implicitly ?

[fitterfizzle]

It is not only about resetting the form, it is about setting the new data. Let's say one component relies on getting the project comment count which is part of the metadata of the project object. When you create a new comment, the mutation invalidates both the comment query and the project query, which currently in the onSuccess of the actual project query simply sets the values anew.

[TkDodo]

sorry this discussion is too involved for me to follow clearly (multiple people, multiple topics, quite theoretical). My suggestion is you create a codesandbox with v4 and I'll take a look and try to come up with an upgrade path to v5.

[kabirsky]

I hoped there would be solutions for one time onSuccess/onError/onSettled properties per fetch, since global function does not really sounds great for my use case - I have a lot of api routes with a lot of unique success and error messages with additional logic, so I pretty much write my code like this:

const queryFn = (props: Props) => {
  try {
    const data = await fetchApi(props);
    const code = data.code;
    
    switch(code) {
    ...
    }
  
    return data;
  } catch (error) {
    if (isAxiosError<BaseAPIResponse>(error) && error.response) {
      const httpCode = error.response.status;
      const data = error.response.data;
      const code = data.code;
  
     switch (httpCode) {
     ...
     }
     switch (code) {
     ...
     }
  
     return Promise.reject(error)
    }
  }
}

const myQuery = (props: Props) => useQuery(queryKey:['some-key', props], queryFn);

It kinda works, but I am not fully satisfied (still best of what I had worked with before).

With what you are proposing you think it's better to just use useEffect for success and error handling? It may actually not sound bad at all - I just never thought about this way of handling things before - although React is running all useEffect twice in development React.Strict mode
Upd. Oops, writing at 5AM was bad idea, useEffect would be exactly like old onSuccess/etc, because it would duplicate for any hook instance, so I do not have solid idea what to do in my particular case

[TkDodo]

it seems a bit like you want to have your cake and eat it, too, because:

• the callbacks should only be called once per fetch
• you want to define them on useQuery level, not globally

those things are kind of at odds with each other, because the hooks are meant to be called multiple times in a component tree. If you call your custom hook 3 times, each one defines a callback for onSuccess / onError, but only one should run. The question is, which one? If they are pure functions that don't close over anything from the component, we could just run any one, and it wouldn't matter. But if they do close over a prop, it would become undefined behaviour. Please see my example from two weeks ago:

#5279 (reply in thread)

You can get that behaviour by passing a function to meta and calling it from the global callbacks, like shown here:

#5279 (comment)

But it's not something I would recommend and it's deliberately not a built-in, straightforward thing to do because it can easily get you into trouble without realising it. So use with caution.

I have a lot of api routes with a lot of unique success and error messages with additional logic

My suggestion would be:

• return custom error message from the API to get access to error.message in a unified way, then display / log that message.
• run additional logic either inside the queryFn if it should be done once per fetch (a queryFn can do more than just one request if it has to ...) or inside the component if it should be done once per instance.

[kabirsky]

Yes, I already saw a variant with meta, and I don't really like it, since meta was introduced for serializable values.
I agree that defining callbacks at useQuery level is kinda dangerous (even though library is already deduplicating queryFn) - it seems you need some kind of addEventListener function for queries, so you can register callbacks to run after success, error, settled, although this thought need more processing before any real discussion.

For now I ended up with one api fetch per query and if I need to have actual values from other query I run await fetchQuery - now I see I should probably use only await invalidateQueries where I can, so I will not throw queryFn functions here and there.

I also experimented with library a bit - I had read-only hooks, that have useQuery without queryFn, and imperative refetch functions, that run fetchQuery - not really great way to deal with things.

[arobbins]

Thanks for taking the time to help answer our migration questions.

I read through your blog post but it's still unclear to me what the potential drawbacks are to using a useEffect on the returned query.data variable.

Prior to query v5, I was using the onSuccess callback to dispatch state changes to various reducers. So I've refactored my code now to use a custom wrapper hook with a useEffect inside like this:

function useGetTemplateQuery() {
	const requestsDispatch = useRequestsDispatch()
	const settingsState = useSettingsState()
	const settingsDispatch = useSettingsDispatch()

	const query = useQuery({
		queryKey: ['templates'],
		queryFn: getTemplate,
		enabled: !!settingsState.template,
	})

	React.useEffect(() => {
		if (!query.data) {
			return
		}

		requestsDispatch({ type: 'SET_IS_BOOTSTRAPPING', payload: false })

		settingsDispatch({
			type: 'UPDATE_HTML_TEMPLATE_DATA',
			payload: query.data,
		})
	}, query.data)
}

I believe this follows the workaround example you provide, but what are the drawbacks to this approach?

[TkDodo]

but what are the drawbacks to this approach?

if the only way to read data is from redux, then yes, there likely isn't a buggy behaviour apart from one extra render cycle. But while you upgrade, you might have places that actually use data returned from useQuery and other places that still read from redux. If that's the case, then there will be one render cycle where those two places will see different data. This can lead to tearing, which is just a small ui inconvenience, but if you trigger other side-effects based on this, it can be a source for serious bugs.

what I would do in your situation is to remove the UPDATE_HTML_TEMPLATE_DATA actions, remove template data from redux and replace all usages that previously grabbed that data via useSelector to have them grab it via const { data } = useGetTemplateQuery() instead.

[arobbins]

Awesome, thanks for the suggestions.

[NathZ1]

I support this decision to remove callbacks, but am struggling converting a codebase to v5 that currently uses onSuccess to post some telemetry (status, response time, etc.) for the various API calls made.

The issue I have is that there is a dynamic property in the telemetry which notes whether the user is waiting for the API call or not - the majority are done in the background, but there are some instances where refetch is called and the user is waiting for the data to be updated. I can see how I can switch to using the global onSuccess callback, passing the query info to the callback via meta, but I can't pass the 'userWaiting' value through meta since it won't change after initially being set.

Is there any way to pass a value that changes through to the global onSuccess callback, without having 2 versions of each query (ie. have the information within the querykey)?

[NathZ1]

Worked it out after reading through some of the other discussions. I can useRef and pass the ref through meta object. Thanks.

[BrendanC23]

Is there a recommended way to update form data from a callback without using onSuccess? I have a select input. When the value is changed, I want to update the form based on the selected value, make a query to fetch some more data, and then update another form field based on the fetched data.

I'm using enabled and nameWasChanged because I don't want the profession to be updated based on the form's initial data. I only want the profession to update if the user makes a new selection.

This is what I'm currently doing:

function Component({ form }: { form: FormReturnType }) {
    const [nameWasChanged, setNameWasChanged] = useState(false);
    const currentName = form.watch("name");
    const currentProfession = form.watch("profession");
    
    const query = useQuery({
        queryKey: ["profession", currentName],
        queryFn: getProfessionForName,
        enabled: nameWasChanged,
        onSuccess: (fetchedProfession: string) => {
            form.setValue("profession", fetchedProfession);
        }
    });

    return (
        <Select
            label="Name"
            options={[]}
            onChange={(newName: string) => {
                form.setValue("name", newName);
                setNameWasChanged(true);
            }}
        />
    );
}

Also, I'm actually using TRPC to handle the queries, so it's actually

const query = trpc.profession.useQuery(currentName, { /* options */ });

which means that getProfessionForName doesn't actually exist and so I can't move the form.setValue() call from the onSuccess into it.
Sorry that this example is pretty contrived.

[weeklyTea]

For people who want to have onSuccess/onError functionality and don't want to pollute the component's code with useEffects next hook may be useful:

type QueryEvents<RespT, ErrT> = {
  onSuccess: (resp: RespT) => any;
  onError: (resp: ErrT) => any;
};

function useQueryEvents<RespT, ErrT>(query: UseQueryResult<RespT, ErrT>, callbacks: Partial<QueryEvents<RespT, ErrT>>) {
  const { onSuccess, onError } = callbacks;

  React.useEffect(() => {
    if (query.data && onSuccess) {
      onSuccess(query.data);
    }
  }, [query.data, onSuccess]);

  React.useEffect(() => {
    if (query.error && onError) {
      onError(query.error);
    }
  }, [query.error, onError]);
}

With this hook onSuccess and onError callbacks can be defined in a more elegant way:

const MyComponent: React.FC = () => {
  const userQuery = useQuery(...)
  useQueryEvents(userQuery, {
    onSuccess: (user) => console.log('User has been fetched: ', user),
    onError: (err) => console.log('An error happened:', err.message),
  })

  return <div>
    ...
  </div>
}

[overflowy]

This is not going to work correctly, because you're passing onError and onSuccess to the dependency array of their corresponding useEffect, and on the first re-render, the callbacks are going to change their references, which means that the useEffect will trigger if query.data and query.error are already cached (which is the default behavior of queries). Basically, after the first successful query, all successive fetches will trigger onSuccess immediately (before getting the result).

This could be a better solution:

import type { UseQueryResult } from "@tanstack/react-query";
import { useEffect, useRef } from "react";

type QueryEvents<RespT, ErrT> = {
  onSuccess: (resp: RespT) => unknown;
  onError: (resp: ErrT) => unknown;
};

function useQueryEvents<RespT, ErrT>(
  query: UseQueryResult<RespT, ErrT>,
  callbacks: Partial<QueryEvents<RespT, ErrT>>
) {
  const { onSuccess, onError } = callbacks;

  const onSuccessRef = useRef(onSuccess);
  const onErrorRef = useRef(onError);

  useEffect(() => {
    onSuccessRef.current = onSuccess;
    onErrorRef.current = onError;
  }, [onSuccess, onError]);

  useEffect(() => {
    if (query.data && onSuccessRef.current) {
      onSuccessRef.current(query.data);
    }
  }, [query.data]);

  useEffect(() => {
    if (query.error && onErrorRef.current) {
      onErrorRef.current(query.error);
    }
  }, [query.error]);

  return {
    data: query.data,
    error: query.error,
    isFetching: query.isFetching,
    refetch: query.refetch,
  };
}

export { useQueryEvents };

[paveltyurikov]

You violate the O-principle of SOLID principles. And you offer solutions that violate the KISS principle. You have solved a rare problem and broken the most convenient API. In a normal world, this could not happen, because it is hard to imagine that this could happen in professional communities.

[joseppu]

go
https://x.com/TkDodo/status/1817121500668784733

[kaueburiti]

This was one of the most inconvenient changes that I had ever seen.

[paustint]

The people in the ivory tower know what is best for everyone....

[Fgabz]

In my usecase, we're using react-query with MVVM architecture.

We changed this way when migrating to last version of react-query

export default function HomeViewModel() {
  const { user } = useUserStore()
  const storyRepository = useMemo(() => createUserStoryRepository(createUserStoryService()), [])
  const fetchAllUserStoryUseCase = useMemo(
    () => createFetchAllUserStoryUseCase(storyRepository),
    [storyRepository],
  )
  const homeViewState = useUserStories()

  function useUserStories() {
    const userId = user?.userId
    const { data, error, isLoading, isError } = useQuery({
      staleTime: 3 * 60 * 1000,
      queryKey: ["user-stories", userId],
      queryFn: () => fetchAllUserStoryUseCase.execute(userId!),
      enabled: !!user?.userId,
    })

    // Handle loading state
    if (isLoading) {
      return { loading: true }
    }

    // Handle error state
    if (isError && error) {
      console.error(error)
      return { loading: false, storyList: [] }
    }

    // Handle successful data fetching
    if (data) {
      const formattedData = data.map((story) => ({
        id: story.id,
        title: story.title,
        coverImageUrl: story.cover,
      }))

      return {
        loading: false,
        storyList: formattedData,
      }
    }
  }

  return {
    homeViewState,
  }
}

[paustint]

so much boilerplate..... Also, why not move data.map((story) => ({ into a select callback?

[cgatian]

I'm curious to understand why the QueryCache doesn't offer a way to subscribe to errors through type=errror?

[TkDodo]

There is no "error" event because we just "update" the query to put it into error state. In the update event, the passed action will have type: 'error' and the query will be in error state.

[cgatian]

Thank you!

[Hulkmaster]

late to the party, but still:

Why not add new callbacks: "onCacheSuccess" and "onCacheError"?

no misleading, better API, does not require write same thing but manually and reuse query client everywhere

[Hulkmaster]

so what i would propose - to have shared promise between all instances, which would share some kind of promise callback, which we can utilize, assuming same queryKey would result in same promise

[TkDodo]

no, that's not why the callbacks were removed.

to have shared promise between all instances

we do have that

which would share some kind of promise callback

we don't have that on component level (because they could close over values, thus can't be "shared"), but we have it on cache level.

[Hulkmaster]

no, that's not why the callbacks were removed.

i'm a little bit confused

https://tkdodo.eu/blog/breaking-react-querys-api-on-purpose#the-callbacks-might-not-run

Because onSuccess is not called again, so no dispatch happens. The parts of our App that use data from useTodos will see the correctly filtered values, but the parts that read from redux will not.

it feels literally like the example i've mentioned

we do have that

how do i access it?
or that is what you meant with:

we don't have that on component level (because they could close over values, thus can't be "shared"), but we have it on cache level.

here is potential problem i see:
#5279 (reply in thread)

If you render this component twice:

<Example id={1} />
<Example id={2} />

and it fails, what would you expect your error toast to say? The good news is, it would only be one toast (not two, which was a problem with the previous callbacks), but the bad news is: we can't tell which one it will be.

why would it be called only once?
the cacheKey is the same, true, but callees are different, thus "query object" must be different, thus should be called N times

because if its not - then approach to move "callback based on promise" to queryCache will not work?

[TkDodo]

it feels literally like the example i've mentioned

in the example, it's not called because staleTime is set and the key changes. Your example is all the same key.

here is potential problem i see:

This is taken out of context, where we're talking about meta as a function. meta exists on the query, so there can only be one per key, but in the example, it closes over different values.

then approach to move "callback based on promise" to queryCache will not work?

I don't even know what you want to achieve so this discussion is not very fruitful and feels a bit arbitrary. It's best if you show a concrete sandbox with an example of something that doesn't work for you.

[Hulkmaster]

in the example, it's not called because staleTime is set and the key changes. Your example is all the same key.

my bad, i forgot to mention we have staleTime and gcTime 30mins, because we want to use query as reactive storage

but with staleTime then my example is correct?

This is taken out of context, where we're talking about meta as a function. meta exists on the query, so there can only be one per key, but in the example, it closes over different values.

got it! thanks

I don't even know what you want to achieve so this discussion is not very fruitful and feels a bit arbitrary. It's best if you show a concrete sandbox with an example of something that doesn't work for you.

First of all - thank you a lot for your time!
I'm very thankful

If you have time, we could schedule a call (so i could show you problems in our project directly) :)

i'm out of time right now, but i'll try to come up with specific examples and show you how i would use callbacks

[offical-LuckyPari]

Interesting article! I really liked how you described the process of rethinking React Query’s API. Deliberately breaking backward compatibility for the sake of improvements takes courage, but it also shows your commitment to the library’s long-term evolution.

[offical-LuckyPari]

It’s great to see how much you focus on transparency in development. Transitioning from an RFC to a blog post indeed helps address many questions and fosters open dialogue. Looking forward to seeing how the community reacts to these changes and the feedback that follows.

[offical-LuckyPari]

Your approach to intentionally changing the API is inspiring. I agree that sometimes you need to “break” the familiar to make a product stronger. A great example of how the community can benefit from such changes. Best of luck with React Query’s growth!

[paustint]

git diff --shortstat
222 files changed, 5364 insertions(+), 5461 deletions(-)

wow.... that was a PAINFUL solid 9 hours just to get the codebase to a buildable state. Now we get to spend a ton of time trying to find bugs that I am sure were introduced. 🤯😭

The code mods are 80% useless as it stripped all the types from useMutation<> which broken type inference, which I had to go back and fix on every occurrence in the codebase.

Renaming isLoading to isPending for mutations required a lot of manual fixing.
And it would have been nice if the codemod could have handled queryClient.invalidateQueries - that was an easy fix but annoying.

Every single file that touched react query in any way required manual modifications - aside from change in function signatures and the removal of isInitialLoading, I feel that all the other breaking changes were made out of philosophical reasons alone and should have just been left as-is since they really weren't causing any harm. So what if some people might have made bad decisions with the design - it was simple and intuitive (specifically onSuccess and onError)

Don't get me wrong, I appreciate the work from the maintainers ❤️ - but migrating to v5 was one of the most painful 🤕 upgrades I have ever had to go through. I would really appreciate more stability even across major versions...

On a positive note, the new devtools experience is slick and I look forward to trying out useSuspenseQuery and potentially the queryOptions stuff.

[tjdraper]

Couldn't have said any of this better myself.

I spent 3 days trying to update from v4 to v5 and I've finally just canceled the update because MY WORD.

I realize that this is a good indication that we let the tendrils of a third party api way too far out into the app, and that's on me and my team. A good lesson to relearn. We try avoid "not invented here syndrome" on my team, but this is definitely a good kick in a direction toward that thinking.

It's unfortunate because we really enjoyed using v4 when we were writing the app.

The amount of re-writing necessary for us to go from v4 to v5 is mind-boggling. The amount of bugs we'd introduce into our stable, flagship product for an update to a 3rd party package is untenable and a waste of time.

I realize breaking changes are inevitable, but NONE of these breaking changes were necessary, and that indicates a willingness to tinker and break things, a lack of respect for long-time users, and a shortsightedness that does not bode well for future stability.

We've been using SSR on our newer apps we've been writing, which obviates the need for ReactQuery. And if we're going to be re-writing stuff anyway, I'd rather rewrite into an SSR pattern than re-write from v4 to v5 of ReactQuery.

[omermizr]

I migrated a mobile app and a web app fairly quickly, probably half a day each. I'd say they were both medium projects, but I don't think the migration process would've taken much more for a larger project. It wasn't too painful TBH.
Maybe it's because we stopped using onSuccess/onError a while back (which for the record I think is a good move considering the previous behavior).
I agree the codemods didn't help at all, and the change to invalidateQueries probably took most of the time, although I did manage to speed this up using some clever find & replace.

The renames were by far the easiest part. Here's what I did for anyone stumbling upon this (I was using vscode, and I assume you use TS):

1. rm -rf node_modules
2. Make a new file that's part of your source (included in your tsconfig) with this inside:

declare module "@tanstack/react-query" {
  export function useQuery(): { isLoading: boolean; isInitialLoading: boolean };
  export function useMutation(): { isLoading: boolean; isInitialLoading: boolean; };
}

Now TS doesn't know tanstack query's type definitions, but it does know yours.
3. Run Rename Symbol (F2) on isLoading, change it to isPending
4. Run Rename Symbol (F2) on isInitialLoading, change it to isLoading
5. Delete the file. You can also reinstall node_modules

I hope this helps people, I definitely would've gone crazy if I had to manually rename that stuff

[jhnns]

@omermizr Wow, that's a pretty slick trick 🤯 👍

[joshuajaco]

So its been 2 years, can we agree this was a stupid change and roll it back?

[sainijagjit]

Please Bring it back !!

[knajjars]

big +1

[TkDodo]

Not sure what you think you’re achieving with this? If you haven’t understood by now why those callbacks are a bad idea, I can’t help you. Fork and add the callbacks back if you want.