onSuccess doesn't get called when query is retrieved from cache

[georgiosd]

Hi!

I used apollo previously on a client project as per the client's request and I really regretted ever doing that. The experience was dreadful, especially when dealing with the cache.

react-query which I then used by my own free will on another project :D has been a breath of fresh air.

However, I do find strange that the onSuccess callback is not getting called when the result is retrieved from cache. It's a bit limiting when you try to use onSuccess and onError to set state.

Could we add that in somehow?

PS: I understand that it's expected behavior based on what the documentation says - but it could be clearer in the docs.

[ryantomaselli]

I would also like to set state onSuccess even when the data is cached.

I'm setting state such that I can differentiate between resting, loading, fetching more and filtering...if I don't get a callback when the query hits the cache I can't set the state back to resting.

I should add that I too am enjoying hooking things up with react-query :)

[christopher-caldwell]

@TkDodo Your explanation was very helpful. Thanks! I had the same question, and was able to work it out based on your answers.

[aakashsrdev]

Thanks @TkDodo

[varian97]

@TkDodo Sorry to continue this thread 4 years later, but I meet the same problem today.
In my case, I have an API to retrieve shipping fee from the backend, so I use useQuery for that. When the query succeed, I want to store it to redux. So I use onSuccess for that

const dispatch = useDispatch()
const { isLoading, isFetching } = useQuery(['shipping-fee'], loadShippingFeeAPI, {
  onSuccess: (res) => dispatch(setShippingFeeData(res))
})

However the issue arise if the customer go to the other page (ex: they press back button) when the API call still not finished, let me breakdown it into steps:

1. Customer open the checkout page, the useQuery for loading shipping fee called, the API call to backend called
2. Before the api call finished, customer press back button, going to the previous page (let say the product detail page)
3. Customer now on the product detail page, the API call finished with status code 200
4. Now at this state, the redux still empty, I assume because onSuccess not called. But the shipping fee query already cached by react-query
5. When customer go to the checkout page again, the UI not updated with the shipping fee info because it reads the data from redux. useQuery also does not call the API again because it is already in the cache

I do solve this problem by using useEffect on the client component, but the questions are:

1. Does this happened because onSuccess / onError does not called when user already changing page? why the data cached inside react-query but the code that dispatch data to redux not called?
2. What are the real use case of onSuccess / onError then? Everything can be put inside useEffect, including showing error toast / success toast

Thank you

[TkDodo]

When the query succeed, I want to store it to redux

That's your problem. You don't want that. Don't do this.

When the query succeed, I want to store it to redux

There are none. We have removed those callbacks in v5.

[varian97]

Thanks @TkDodo
I read your article https://tkdodo.eu/blog/breaking-react-querys-api-on-purpose and it shows exactly the same problem that I wrote.

[cliffordfajardo]

Adding my story as a data point in case others stumble upon this behavior as well.
My code was relying on the onSuccess callback of a useQuery hook to call a setState. It looked something like this:

const SomeComponent = ({ children }: { children: ReactNode }) => {
  const [renderItems, setRenderItems] = useState([])
   useQuery(..., {
    onSuccess(APIData){
       // 🔴  onSuccess will not trigger if data is grabbed from cache
       const renderData = util.transform(APIData); // all this does is transform the API data into another format
        setRenderItems(renderData) // so of course this doesn't trigger and my component below won't re-render with new daat
    }
  })
  
  return (
     <SomeListComponent items={renderItems} />
  )
}

When I enabled caching (this applies to the in memory cacheTime and persistQueryClient approaches) I was confused as to why my onSuccess callback wasn't getting called. This was problematic because I needed my UI to re-render.

Solution:

• remove your onSuccess usage and derive your data instead.

The key thing here is that since I am no longer using OnSuccess, I am able to get the data from the cache.

const SomeComponent = ({ children }: { children: ReactNode }) => {
  const {data: APIData} = useQuery(...)  
  const renderItems = APIData ? util.transform(APIData): []; ✅  works
  return (
     <SomeListComponent items={renderItems} />
  )
}

[TkDodo]

we will make it more clear in the docs that onSuccess is tied to data fetching, like onError. To perform side effects whenever data changes, a useEffect is still the best approach.
I also think it’s sub-optimal to do data transformations and then store the result in state, as this violates a principle of react to only store the most minimal state and derive the rest. The select option offered by RQ is perfect for that scenario.

[cliffordfajardo]

Thanks - I’ll start looking at select for the data transformation 👀

[davit-b]

Yep, ran into this same problem and took an hour to realize that onSuccess is NOT called on cache hits.

[donysukardi]

I'm hoping to have some callback regardless of whether it's freshly fetched or retrieved from cache as well. My use is case is to trigger some side effect, e.g. redirect with some default params

[cliffordfajardo]

I've come back to this thread a few times, because of I assumed onSuccess would get called if react-query has the data
its not intuitive that onSuccess doesn't get called when the data is in the cache. onSuccess is onSuccess lol! 😄 no matter if the data came from a cache or not

This is so weird to use useEffect whenever we need to update some state, based on data we received (from cache or network). I also prefer if onSuccess gets called each time data were retrieved doesn't matter from where

+1 to previous thread reply

[TkDodo]

I also prefer if onSuccess gets called each time data were retrieved doesn't matter from where

I thought about this onDataChanged callback some more and I don't think we'll do it. Let me explain:

When data is read from the cache, you get it back synchronously. There is no way for us fire a "side-effect" here, apart from manually triggering a useEffect. However, useEffect is react specific, so we would need to re-create this behaviour for every framework adapter. And even if we do this, the root problem with effects still persist that there will be one render cycle in between where data will be out-of-sync. So you will have data returned from useQuery already be the "new" data read from the cache, but your local "copy" of it will still hold the "old" data.

onSuccess works now because data fetching already doesn't happen during rendering, so whenever data comes back, we can trigger side-effects.

FWIW, I think this is an anti-pattern that should be avoided:

const [renderItems, setRenderItems] = useState([])
useQuery(..., {
  onSuccess(APIData){
    setRenderItems(APIData)
  }
})

it duplicates server state into client state which shouldn't be necessary. This sentence says it best:

this is so weird to use useEffect whenever we need to update some state, based on data we received (from cache or network)

Imo, you should be feeling weird, because it is a weird thing to do. State should have a single source of truth, and further state can be derived. Having effects that sync state is, again, not a good idea. If we provide an abstraction to make you feel less weird, we would further encourage doing this, even though I feel strongly that you probably shouldn't. At least with a useEffect in user-land, you'll see explicitly what you are doing.

I would like to encourage everyone in this thread to read the excellent You Might Not Need an Effect section in the new react docs. If you really must do this, the docs even say that setting state during rendering would be preferred. This is the equivalent of the class based getDerivedStateFromProps:

const [renderItems, setRenderItems] = useState([])
const { data } = useQuery(...)

if (data !== renderItems) {
  setRenderItems(data)
}

The docs are still pretty clear that you probably don't want this though:

Although this pattern is more efficient than an Effect, most components shouldn’t need it either. No matter how you do it, adjusting state based on props or other state makes your data flow more difficult to understand and debug.

I know that the example is contrived and you're all probably not setting data 1:1 into local state, but still:

• if the manipulation is synchronous, state should be derived during render (more on this here: https://tkdodo.eu/blog/dont-over-use-state)
• if the manipulation is asynchronous - it should be another query, from which you can then again synchronously derive things.
• if you need data to be put into state as an "initial value" for further updates by the user, you also don't want an effect, because that would potentially overwrite user-input. There are multiple ways to go about that, which I'm describing here: https://tkdodo.eu/blog/react-query-and-forms

[ShamansCoding]

FWIW, I think this is an anti-pattern that should be avoided:

const [renderItems, setRenderItems] = useState([])
useQuery(..., {
  onSuccess(APIData){
    setRenderItems(APIData)
  }
})

And if I need to display the data filtered by some condition along with the original data? For example, to show a general list in one part of the interface, and the same list in the other, but filtered by belonging to the user who is viewing the page.

const [itemsOfCurrentUser, setItemsOfCurrentUser] = useState([])
useQuery(..., {
  onSuccess(APIData){
      setItemsOfCurrentUser(APIData.filter(..some filter by user condition, may be by current userId...)) 
  }
})

I understand that we can do the same thing in other ways, but in this particular situation, it does not look like bad practice to do so.

And there are many such situations. For example, I can fire some kind of event to my clickstream database based on what is currently displayed on the user's page, from cache or not. Doing this in onSuccess looks like a very convenient solution.

Don't get me wrong, react-query is by far the best library I've come across for working with back-end data. But things like working with onSuccess do cause some frustration.

In general, the scope of onSuccess is extremely narrow at the moment and I see it more like onServerDataLoadSuccess.

[TkDodo]

This isn't even about react-query. Your above example is really just:

const { data } = useQuery(...)
const itemsOfCurrentUser = data.filter(...)

this is a prime example of what I meant by derived state. It's all over the docs page I linked and the blog-posts I posted. You can wrap this in useMemo if you don't want to execute it during every render, but you should execute it during render, because that's where data should be derived. Derived state isn't state. With this, the itemsOfCurrentUser will always be in-sync with the server state returned from useQuery.

In general, the scope of onSuccess is extremely narrow at the moment

I agree. I'm pondering to remove it 😅

[georgiosd]

+1 on removing it completely 😆

[miuan]

I see the problem when using the onSuccess to update redux-store

for ex: we have switch A x B, what call some endpoint with A or B, and response we consume like const { data } = useQuery('some-ep', {A|B}) bud for some components we also push to the store... so in case if you switch from A to B, all good , but if you switch back to A, then we have correct data, but store wasn't updated

only solution I have in mind I have to use useEffect to check if data was updated, but that is not good approach , duple every useQuery with useEffect

[TkDodo]

yes, the preferred solution is to not sync with redux at all :)

[miuan]

:) thank you for very quick answer, easy to say, harder to do :)

let say you have two components:

1. have Switcher A / B with show result - here is easy you can call useQuery directly base on user input and show the result
2. doesn't have Switch but only show the result , so you have two option somehow propagate user setting to call useQuery also here, or propagate data from useQuery called in first component to second, booth solution are harder by how far in DOM these two components are each other (ex: viewer is in header , switcher is deeply in view)

I very like concept of useQuery, but I just think we should consider another solution for retrieving data from server than useQuery in our app :(

[TkDodo]

This is a client state problem, not a server state one. Instead of putting the result from useQuery into redux, you want to put the user input itself into redux. This will give you something like:

const useResult = () => {
  const userInput = useSelector(state => state.userInput)
  return useQuery({
    queryKey: ['result', userInput],
    queryFn: () => fetchResult(userInput),
    enabled: !!userInput
  })
}

Now you can call useResult() wherever you want, your client state lives in redux, your server state lives in react-query, and you have no state duplication problem. Every time you update userInput in redux, your server state will automatically be up-to-date with that input.

[miuan]

I have also this in mind, but that is in my opinion more complicate regarding how far our app is already developed, but thank you very much

[ppYoung]

a common situation is: we use a store to share data among components, the store data comes from RQ and can be changed by user action

const [complexData setComplexData] = useAtom(complexDataAtom)

const { data } = useQuery(…, {
    onSuccess(apiData) {
        setComplexData(apiData)
    }
})

// component A
setComplexData((draft) => draft.a = someUserValue)

// component B
setComplexData((draft) => draft.b = someUserValue)

but if switch the query key a -> b -> a, the store data cannot be set to correct data with queryKey a, still keep the previous data from queryKey b, the only way to keep the data update is useEffect, and unlike your demo about useResult when complexData changed to refetch data, no, i don’t need to refetch data when store change, the only solution is:

useEffect(() => {
    setComplexData(data)
}, [data])

it’s so weird, i just need a callback when data fetched, no matter from server or from cache🥲

[TkDodo]

useEffect solves that. However, I truly believe that you shouldn't sync state from react-query to another state manager. That's the root cause of the problem, and it will yield other problems, e.g. user input being overwritten from background refetches.

[georgiosd]

It's amazing to me that 2.5 years later we're still discussing the same thing over and over 😆

It's also amazing to me that you @TkDodo (still) have the patience to respond to people and haven't closed the thread!

[aakashsrdev]

Thanks @TkDodo @georgiosd @ShamansCoding for discussing this topic in so much detail. This use case of onSuccess was worth discussing. I've not got this particular use case yet, but I know now how to handle it now. 😃

[TkDodo]

It's also amazing to me that you @TkDodo (still) have the patience to respond to people and haven't closed the thread!

closing discussions is a new thing, but I'll make use of it now. onSuccess will be removed in v5 so we won't have that problem:

• RFC: remove callbacks from useQuery #5279

[aakashsrdev]

@TkDodo then what will replace this onSuccess callback ?

[TkDodo]

have you read the linked blogpost?

[aakashsrdev]

Sorry, got the point. Thanks for detailed blog

[davit-b]

Glad you are removing onSuccess - it's a courageous DX decision. So many people, me included, mis-used onSuccess for side effects.

And all the damn tutorials online teach you to use onSuccess for side-effects too :(

Thanks for having the courage to make this change.

[ppYoung]

亲，你的邮件东阳已经收到了