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.

[mrlectus]

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!

[dekuhl]

Sorry if I am missing something but I couldn't find a suggested pattern for my use case. here I saw that we could use the local onError callback to handle errors for a specific query. I have a use case where the way the error is handled depends on the response like this:

  useQuery({
    queryKey: ['todos'],
    queryFn: fetchTodos,
    onError: (error) => {
      if (error.status === 400) {
        toast.error('Something went wrong');
      } else {
        toast.error('Something else went wrong');
      }
    }
  });

I only want this logic to apply to this query and none of my other queries. I already have a global onError function using the QueryCache. I have a lot of queries with error handling specific to that API call, so I don't want to add logic to the QueryCache for one specific query. I would like to keep the error handling logic in the same place as the hook. I don't think the meta object can help here either because I am dependent on the response. Should I be handling this error in the queryFn?

[dekuhl]

That's what I am trying to avoid so I don't have a giant error handling switch or if/else block:

const queryClient = new QueryClient({
  queryCache: new QueryCache({
    onError: (error, query) => {
      switch (query.queryKey) {
        case 'key1':
          toast.message(`handle error 1 ${error.message}`);
          break;
        case 'key2':
          if (error?.data?.message.includes('SPECIAL_CASE')) {
            toast.message(`handle error 2 case 1 ${error.message}`);
          } else {
            toast.message(`handle error 2 case 2 ${error.message}`)
          }
          break;
        default:
          toast.message(`handle default ${error.message}`);
      }
    }
  })
})

If I have 30 services/custom hooks that need special error handling this could get messy quickly. I use a lot of services with some of them not designed well, so we have to do a lot of error handling on the front end. All of the logic for a service is handled in my custom hooks. It doesn't feel great to put logic for a service in the QueryClient to handle errors.

[TkDodo]

you can always spawn a useEffect in your component if you don't like the other options. That is literally the same thing as the old onError callback:

const { error } = useQuery({
  queryKey: ['todos'],
  queryFn: fetchTodos,
});

React.useEffect(() => {
  if (error) {
    if (error.status === 400) {
      toast.error('Something went wrong');
    } else {
      toast.error('Something else went wrong');
    }
  }
}, [error])

[atropos0902]

bye react-query

[divmgl]

This is wild to me, no? Why remove an already working part of the framework? Now anyone who wants to use newer versions of tRPC are going to run into this massive codebase-wide problem.

[TkDodo]

Why remove an already working part of the framework?

because it wasn't working.

[phiter]

The main reason we use react-query anyways, apart from caching, is to avoid having to use useEffect, like for api calls on mount and then having to do setState. I understand that the hooks might not be necessary but they are a convenience, just like react-query.

What will happen is people will eventually have to do workarounds to maintain the availability of those callbacks, for example something like this:

import { UseQueryOptions, useQuery } from '@tanstack/react-query';
import { useEffect } from 'react';

interface Callbacks {
    onSuccess?: (data: any) => void;
    onError?: (error: any) => void;
}

export const useQueryWithCallbacks = <T>(options: UseQueryOptions<T> & Callbacks) => {
    const result = useQuery(options);

    useEffect(() => {
        if (result.isSuccess && options?.onSuccess) {
            options.onSuccess(result.data);
        }
    }, [result, options]);

    useEffect(() => {
        if (result.isError && options?.onError) {
            options.onError(result.error);
        }
    }, [result, options]);

    return result;
};


...

const { data, isLoading, isError } = useQueryWithCallbacks(
        {
            queryKey: ['users'],
            queryFn: listUsers,
            onError: () => toast.error('error'),
            onSuccess: () => toast.success('success')
        }
    );

This makes V5 behave similarly to V4, but then you don't have to add an extra useEffect to your component (which is what you probably try to avoid anyways by using react-query).

[TkDodo]

couple of issues with this abstraction:

• options are probably an inline object, so your effect might trigger to often. Have a look at the latest ref pattern to potentially work around that
• result returned from useQuery is not referentially stable, so you'd definitely want your dependency array to only contain data, not the whole result object.
• the onSuccess callback will be invoked whenever data changes, even if that didn't happen due to the queryFn running (e.g. if you put data manually in the cache). But that's generally a good thing.

onError: () => toast.error('error'),
onSuccess: () => toast.success('success')

maybe you're just using toasts as an example here, but I think this is fundamentally wrong. Custom query hooks are meant to be called whenever you need access to data, so you can have calls to useTodos() multiple times in your application. If that's the case, you'd get multiple toast notifications for every successful / erroneous query. But I've said that in the article a bunch of times, so I guess you're all aware of that and just accept that.

[phiter]

result returned from useQuery is not referentially stable, so you'd definitely want your dependency array to only contain data, not the whole result object.
the onSuccess callback will be invoked whenever data changes, even if that didn't happen due to the queryFn running (e.g. if you put data manually in the cache). But that's generally a good thing.

@TkDodo Ahh yeah, your second and third point are true. The dependencies should be those two you mentioned, that was just a quick attempt at the custom abstraction.

My main pain point with the removal are forms. For example, the pattern our project uses is that we have the originalData and updatedData. originalData comes from the api and updatedData is a clone of the api data that we modify and then we use to compare if the form has changes and that is the value we send to the api to update a certain entity.

You have this blog post: https://tkdodo.eu/blog/react-query-and-forms. This is really well written and there are some things that could work, like having the form be a separate component and only mount the form when the data is available. I actually do need to render the form instead of placing a "Loading" screen on top of it, because I use a loading prop on my input fields to make a skeleton (probably a terrible pattern tbh). But this is all just extra work I feel like.

Custom query hooks are meant to be called whenever you need access to data, so you can have calls to useTodos() multiple times in your application. If that's the case, you'd get multiple toast notifications for every successful / erroneous query.

I don't get this part. Are the current callbacks called even if other queries in the application run? Like if I use useQuery in two separate components that are being rendered, is onSuccess gonna be called whenever any of those two queries run?

[TkDodo]

I don't get this part. Are the current callbacks called even if other queries in the application run? Like if I use useQuery in two separate components that are being rendered, is onSuccess gonna be called whenever any of those two queries run?

Not other queries, but the same query in other components. Consider:

export const useTodos = useQuery({
  queryKey: ['todos'],
  queryFn: fetchTodos,
  onSuccess: () => {
    console.log('onSuccess from useTodos')
  }
})

And now let's use this hook in two components:

function ComponentOne() {
  const { data } = useTodos()
}

function ComponentTwo() {
  const { data } = useTodos()
}

Those two components could be anywhere in our app. Say we use it once in the SideBar to display the amount of todos (ComponentOne), and then once in the content area to display the list (ComponentTwo).

Now we have two Observers for the same QueryKey, and onSuccess will be called for each one of them, so we'll see the log statement twice. If that were to render a toast, we would see two toasts in the UI.

[phiter]

Ahhh I see. That makes sense. Yeah I wouldn't be using onSuccess internally in that function anyways and in the case I mentioned I would use it at the component level. So I would do useTodos({ onSuccess().. }) instead, which would not trigger that behavior.

My main use case for onSuccess internally would be on a mutation to invalidate other queries or update their data.

[TkDodo]

My main use case for onSuccess internally would be on a mutation to invalidate other queries or update their data.

That's a good use-case, and we're not changing that. Only the callbacks on useQuery are removed.

[Johannes5]

So... what do you recommend doing instead? If you simply want to log out every response.

[TkDodo]

So... have you read the blogpost?

Yes, the example is for onError and toasts, but there's also onSettled (which is also called for successful requests) and console.log. And you can also handle this on your "request layer". Like, imagine you wouldn't use RQ, how would you handle it then? Maybe an axios response interceptor (if axios is what you're using) or a ky afterResponse hook. Those things can still be used with RQ ...

[davydof]

hi, what if I need to do some custom actions on different useQuery instances?

e.g.:

useQuery({
  queryKey: ['todos'],
  queryFn: fetchTodos,
  onSuccess: () => {
    // send bi
  },
  onError: (error) => {
    // send sentry error
  }
});

useQuery({
  queryKey: ['todos2'],
  queryFn: fetchTodos2,
  onSuccess: () => {
    // send bi
    // do something else
  },
  onError: (error) => {
    // send sentry error
    // do something else 2
  }
});

should I do something like?

useQuery({
  queryKey: ['todos'],
  queryFn: fetchTodos.then(
    () => {
      // send bi
    },
    () => {
      // send sentry error
    },
  ),
});

thanks

[davydof]

@TkDodo

[lotrgollum87]

@TkDodo Is there no good way to handle errors differently for each useQuery instance?

[TkDodo]

what if I need to do some custom actions on different useQuery instances?

imperative actions that should be different for each useQuery instance were the one thing the callbacks were somewhat good at. There was only one good example I have seen for this, which is imperatively animating an element when an error occurs. For this, you now have to useEffect.

should I do something like?

sendBi and sendSentryError are things that you usually want to define globally. Even if you only want some errors to be tracked in sentry, setting a meta field and doing the actual handling globally is usually better. No idea what "do something else" would be in practice. If there is a good use case, I'd like to hear about it. There might be some that I haven't seen in the last 9 months since we announced the change and the thousands of complaints we got because we removed the callbacks :) Like I said above, the vast majority of those were suboptimal use-cases of the callbacks, and a lot of them were flat out mis-usages. So yes, if your use-case belongs to the minority, you have to refactor to useEffect for the greater good. useEffect is not that evil - and having an abstraction just so that you don't have to write useEffect is worse if that abstraction isn't good, like the callbacks were.

Is there no good way to handle errors differently for each useQuery instance?

if it's declarative, use the error field returned from useQuery. Otherwise, also, see above.

[vctqs1]

Hi @TkDodo might be late but i'd like to post question here to be prepare for upcoming change in v5
In my project, there are many cases like we just return the nested object of a original data. We usually use onSuccess todo that

Example

useQuery({
  variables,
  onSuccess(data) {
    return data.conversations.lists || []
  }
})

Then to follow upcoming changes can we use select function to alternative

useQuery({
  variables,
  select(data) {
    return data.conversations.lists || []
  }
})

Or do some transfrom(data) to another fromat.

There is any things with negative effect here?
Also is it okay to call a async function inside select?

I read the blog and see that we can replace it with useEffect but it might there's more line of codes that the simply thing I need.

Thank you in advance.

[TkDodo]

returning something from onSucess doesn't do anything. onSucces has never been a place to do data transformations, so this code never worked / did anything meaningful:

  onSuccess(data) {
    return data.conversations.lists || []
  }

is it okay to call a async function inside select?

no. If your transformation is async, do it in the queryFn.

https://tkdodo.eu/blog/react-query-data-transformations

[vctqs1]

@TkDodo ah sorry I was not ask the clearly question I mean

useQuery({
  variables,
  onSuccess(data) {
    const result = data.conversations.lists || [];
    asyncMethod(result)
  }
})

That why I ask to use async method in select. But your answer already include it
I got it now thank you

[aakagi]

Here's a hack I'm using to get around this update:

// App.tsx or wherever you define your `<QueryClientProvider client={queryClient}>`

const queryClient = new QueryClient({
  queryCache: new QueryCache({
    onError: (error, query) => {
      captureException(error);

      if (query.meta?.onError) {
        return query.meta.onError(error, query);
      }
    },
  }),
});

// Example query hook that wraps `useQuery`

export const useExampleQuery = () => {
  return useQuery({
    queryKey: ["exampleQuery"],
    queryFn: () => fetchExampleData(),
    meta: {
      onError: (error, query) => {
        toast.error("Something went wrong :(");
      },
    },
  });
};

// react-query.d.ts

import "@tanstack/react-query";

// Extends the QueryMeta interface from react-query to include our custom notification message types
declare module "@tanstack/react-query" {
  export interface QueryMeta {
    /**
     * Exposes a `meta.onError` event to `useQuery` options.
     */
    onError: QueryCacheConfig["onError"];
  }
}

I think this pattern makes a lot of sense... I mostly just want to still be able to present a custom toast depending on the error in my useQuery wrapper hooks.

Is there any danger to using a pattern like this?

[TkDodo]

Is there any danger to using a pattern like this?

I wouldn't recommend putting functions into meta, because you can run into stale closure problems if your function closes over anything, like props:

function Example({ id }) {
  const { data } = useQuery({
    queryKey: ["exampleQuery"],
    queryFn: () => fetchExampleData(),
    meta: {
      onError: (error, query) => {
        toast.error(`Something went wrong for ${id} :(`)
      },
    },
  })
}

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. There is only one meta on the query (not one per observer). The queryFn itself has pretty much the same problem, but we have lint rules to enforce that this doesn't happen. So if you really want that, why not just:

queryKey: ["exampleQuery"],
queryFn: async () => {
  try {
   return fetchExampleData()
  } catch(e) {
    toast.error(`Something went wrong for ${id} :(`)
    throw e
  }
}

At least now, our lint rule would tell you that you have to include id into the queryKey.

---

Additionally, meta is included in the (de)serialization process of hydration, so if you put something in there that is non-serializable, it can't be stored / transferred.

So for the case you've shown, I'd do:

meta: {
  errorMessage: "Something went wrong :("
}

and put the call to toast.error into the global onError callback.

[jyuloeng]

function Example({ id }) {
  const { data } = useQuery({
    queryKey: ["exampleQuery", id],
    queryFn: () => fetchExampleData(),
    meta: {
      onError: (error, query) => {
        toast.error(`Something went wrong for ${id} :(`)
      },
    },
  })
}

Is the above problem solved?

[TkDodo]

well yes, I guess the queryFn will also need to make use of the id:

queryFn: () => fetchExampleData(id),

the problem is there is nothing that forces you to add things to the QueryKey that you use in onError of meta. So it's easy to get wrong, that's all.

[jyuloeng]

haha, look like this, thanks~

[vctqs1]

queryKey: ["exampleQuery"],
queryFn: async () => {
  try {
   return fetchExampleData()
  } catch(e) {
    toast.error(`Something went wrong for ${id} :(`)
    throw e
  }
}

This approach works well when we don't need to retry the operation.
However, if we need to retry it, we should be mindful of the potential increase in log entries generated as a result.
Let's say we set retries = 2. So if all the requests failed, it will show 3 messages at all

[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.

[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,
  }
}

[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