React-apollo: Query.onCompleted isn't fired after fetching data from the cache

[elessarperm]

Intended outcome:
I tried to make a side-effect when query result is fetched using <Query onCompleted={...}>

Actual outcome:
onCompleted fired after the real network request, but when I changed variables and then switched them back, query result was returned from the cache and onCompleted handler wasn't executed.
However, it works when I change fetchPolicy to cache-and-network or network-only.

How to reproduce the issue:
Make a <Query> that should log result into console when completed. Run it with some variables,
then change it to cause re-render, then return variables back to make it re-render result from cache.
onCompleted handler won't be fired.
fetchPolicy should be cache-first

Versions
npmPackages:
apollo-boost: ^0.1.10 => 0.1.10
apollo-cache-inmemory: ^1.2.5 => 1.2.5
apollo-client: ^2.3.5 => 2.3.5
apollo-link: ^1.2.2 => 1.2.2
apollo-link-http: ^1.5.4 => 1.5.4
react-apollo: ^2.1.9 => 2.1.9

If it's not a bug, but a feature, can you explain how could I perform a side-effect after every query completion regardless of it uses cache or not?

[olistic]

I'm trying to update the state of my form with the result of a query, and I'm experiencing the same problem. onCompleted is not fired when the data is retrieved from the Apollo cache. This is an important issue as one cannot rely on the onCompleted callback if data from the cache (which plays an important role in Apollo) does not trigger it.

[Dartv]

I'm facing the same exact issue as @olistic
Can we get this fixed?

[serranoarevalo]

Any workaround for this?

[serranoarevalo]

Adding: fetchPolicy={"cache-and-network"} to the Query Component fixed it for me.

[hxuanhung]

@serranoarevalo are you sure? I've tried but it seems doesn't solve the problem.

[serranoarevalo]

@hxuanhung yup, just tested it again and it still works. https://github.com/nomadcoders/nuber-client/blob/master/src/Routes/EditAccount/EditAccountContainer.tsx#L45

[Dartv]

It works but it's not ultimately a fix. It's a workaround at best and at a cost of extra traffic.

[laspluviosillas]

I'm having similar issues, except nothing resolves mine. onCompleted never fires regardless of what I do. (and yes I tried changing the fetchPolicy to a variety of things)

[Cabezaa]

I'm having the same problems. Has anyone been able to solve this?

[franzejr]

I got the same error. Fixed it for now by using no-cache as policy.

[laspluviosillas]

FYI, for everyone having this issue, you can work around it by managing the query manually through withApollo instead of using the Query react tag. Here's an example.

import React from 'react';
import { withApollo } from 'react-apollo';

class Page extends React.Component {
  constructor(props) {
    super();
    this.state = {
      loading: true,
      data: {},
    };
    this.fetchData(props);
  }

  async fetchData(props) {
    const { client } = props;
    const result = await client.query({
      query: YOUR_QUERY_HERE, /* other options, e.g. variables: {} */     
    });

    this.setState({
      data: result.data,
      loading: false,
    });
  }

  render() {
    const { data, loading } = this.state;
    /* 
      ... manipulate data and loading from state in here ...
    */
  }
}
export default withApollo(Page);

This approach relies on state so does not work with functional components, and is of course much uglier than simply using a Query tag, but if you really need a solution this approach does the job, and does not require something drastic like skipping on caching.

[yantakus]

I can confirm that fetchPolicy='cache-and-network' doesn't always invoke onCompleted callback with this codesandbox: https://codesandbox.io/s/6zqzy802nr

And I totally agree that even if it does work in some cases, this is still a dirty hack rather than solution.

Apollo team, onCompleted should be always fired, that's simple logic.

[hwillson]

Confirmed - repro: https://codesandbox.io/s/50zyl4xqol
We'll get this fixed.

[Dartv]

onCompleted still isn't fired after mutation automatically updates cache

[hwillson]

@Dartv Can you provide a small runnable reproduction that demonstrates this?

[Dartv]

I know i can use onCompleted on mutation, but shouldn't Query onCompleted be also called since the cache was updated?

@hwillson here https://codesandbox.io/s/mm537nmqr9

[vititu]

Now onCompleted is being called on every render. Before it was called once.

That might break the app causing a Maximum update depth exceeded error if try to setSate with the data provided.

[tab00]

I get error "Maximum update depth exceeded. This can happen when a component repeatedly calls setState inside componentWillUpdate or componentDidUpdate." in browser console after updating to 2.2.4, preventing any rendering. The problem didn't occur in 2.1.11. Please fix this problem.

[tab00]

I've just downgraded to 2.1.11 and the component renders correctly.

I have a parent component that passes a function as a prop to a child component. That function calls setState, so that the child can update the parent's state.

[riccoski]

I'm also getting the issue of not being called when getting it from the cache
EDIT:
setting notifyOnNetworkStatusChange={true} works

[reyesreg]

If anyone here is having the same issue for mutations --

I have two identical mutations that call the same openSuccessBanner in onCompleted.
The one that is directly passed works fine.
The one that is passed down twice fails. Such a strange error 😷

I've just chained the function to the actual mutation for the second case.

mutateUpdateInvitation().then(() => {
  this.props.openSuccessBanner();
})

[SeanningTatum]

Now onCompleted is being called on every render. Before it was called once.

That might break the app causing a Maximum update depth exceeded error if try to setState with the data provided.

@tab00 I fixed it by using a PureComponent, because when you update the component React will re-render causing the Query component to run again indefinitely setting the state.

Hope this works for you!

[yantakus]

@the same thing for me. Now it's impossible to use setState inside onCompleted, because the app gets into infinite loop. So I had to get rid of Query component and use graphql HOC to update state with remote data, this seems the only option ATM.

[vititu]

Now onCompleted is being called on every render. Before it was called once.
That might break the app causing a Maximum update depth exceeded error if try to setState with the data provided.

@tab00 I fixed it by using a PureComponent, because when you update the component React will re-render causing the Query component to run again indefinitely setting the state.

Hope this works for you!

@SeanningTatum I believe you are right, might be a good idea. Thanks!

[SeanningTatum]

@the same thing for me. Now it's impossible to use setState inside onCompleted, because the app gets into infinite loop. So I had to get rid of Query component and use graphql HOC to update state with remote data, this seems the only option ATM.

@yantakus I fixed this problem by changing my component into a PureComponent

This works because when you setState it will trigger a re-render because you updated the component - and inside your render function is the Query component. Thus running onCompleted indefinitely.

You should use PureComponent or shouldComponentUpdate if intent to use the Query component. But firing the query manually works too!

[kandros]

it doesn't feel natural onCompleted should mean on query completed not on read from cache a boolean property like onResult (which can be both from cache or network) feels more appropriated

[riccoski]

Is this working for anyone? - I'm still getting the same issue of it not being fired :(

[ridhoq]

I'm still not seeing onCompleted get called when a result is returned from the cache. I verified that this is still occurring by adding fetchPolicy={"cache-and-network"} to my Query component as suggested above and started to see the expected behavior.

[ridhoq]

Somehow notifyOnNetworkStatusChange={true} makes it work as well...which I don't understand. Is that expected behavior?

[kandros]

@ridhoq there are a couple of bugs in fetchPolicy that have unexpected behaviours when mixed with other props

[ridhoq]

@kandros hmm I understand why setting my fetchPolicy would cause onCompleted to be called, but I don't understand why notifyOnNetworkStatusChange would

[daemonsy]

Hi, I have the same issue as @vititu. The use case is that I have to keep track of the data received by the GraphQL query to determine whether to do an action based on it. I guess onCompleted is just one hook, it's impossible to get everyone's use case.

Guessing it's probably a common need to execute handlers based on different events. Not sure if the core team looked at it, but maybe someone with more experience using Apollo can help to move the issue forward by doing a RFC with a list of common event hooks.

[jsslai]

Now it is really difficult to perform side-effects with queried data. I think onCompleted should be called based on fetchPolicy:

• cache-first: Call onCompleted when cached or network data available.
• cache-and-network: Call onCompleted if cached data available and call onCompleted again when network data available.
• network-only: Call onCompleted when network data available.
• cache-only: Call onCompleted when cached data available.
• no-cache: Call onCompleted when network data available.

Maybe the callback should be renamed or add a new callback for this purpose if needed.

[chitswe]

@vititu @yantakus . I use a different workaround. You have to keep reference to completed query result with react state or instance variable. Make sure you only call setState only if current query result is not equal with previous keep result. Here is example.

<Query
onCompleted={data => {
          if ( this.lastFatchedData !== data) {
            setState({..});
          }
          this.lastFatchedData = data;
}
/>

[rhlsthrm]

Had the issue with the callback firing over and over, so I ended up using @laspluviosillas's solution, which worked great: https://github.com/apollographql/react-apollo/issues/2177#issuecomment-415770550

Thank you!

[Maxtermax]

with all respect apollo have really long way to be considere a stable, easy and relieble library, thar arquitecture of pass the fetch layer to a react HOC is a mistake, beacuse limit you to the react lifecycle so is easly fell into a hole of error or glitches, currently the only safe way to set the state after a query without get hit by an error is this way:

// CatchResponse.js
class CatchResponse extends React.Component {
  componentDidMount() {
    this.props.done(this.props.data);    
  }

  render() {
    let { children } = this.props;
    if (children) return children;
    return null;
  }
}

//Main.js
class MainComponent extends React.Component {
  state = {
    value: 0
  }
  render() {
    return <Query
       notifyOnNetworkStatusChange={true}
       query={/**/}
    >
      {
        ({ error, data, networkStatus }) => {
          let loading = !(networkStatus === 7 || networkStatus === 8);
          if (loading) return <h1>loading...</h1>
          if (data) {
            return React.cloneElement(
              <CatchResponse
                data={data}
                done={response => {
                  console.log("response ", response.name);
                  this.setState({ value: 1 });
                }}
              >
                <h1>{data.name}</h1>
              </CatchResponse>, {})
          }
        }
      }
    </Query>
  }
}

Explanation

First is necessary to create a component that will wrap and render our target component, using the
componentDidMount to call a function passed by props called done.

// CatchResponse.js
class CatchResponse extends React.Component {
  componentDidMount() {
    this.props.done(this.props.data);    
  }

  render() {
    let { children } = this.props;
    if (children) return children;
    return null;
  }
}

Be aware of the loading issue for that reason is necessary to add this line in the Query component

   let loading = !(networkStatus === 7 || networkStatus === 8);

And for last use the CatchResponse component to receive to paint the view, note the this.setState inside the done function is now safe to use because with this configuration the react lifecycle is respected, also note React.cloneElement ensure that CatchResponse get remounted and the componentDidMount is called and by side effect the this.props.done(this.props.data); is also called.

return React.cloneElement(
              <CatchResponse
                data={data}
                done={response => {
                  console.log("response ", response.name);
                  this.setState({ value: 1 });
                }}
              >
                <h1>{data.name}</h1>
              </CatchResponse>, 
       {})

[coffeedoughnuts]

FYI: I am able to recreate this issue with "react-apollo": "2.5.5" - notifyOnNetworkChange solves the problem, but obviously not ideal.

Edit: apologies, this comment was for an issue that links to this, but I'll leave it here in case it's relevant

[pfarnach]

Still seeing this problem in react-apollo: 2.5.4. Setting fetch-policy="cache-and-network" to trigger onCompleted works but isn't ideal. @hwillson any updates on this?

[PavanBahuguni]

notifyOnNetworkStatusChange works and doesn't make a network call. Good workaround.

[hamxabaig]

I have also the same problem, onCompleted still not fires on v2.5.6

[garfiaslopez]

onCompleted still not fires on v2.5.8. why is this closed?

[jasonpaulos]

@garfiaslopez or @hamxabaig, can you provide a runnable reproduction that shows this issue?

[DominicTobias]

onComplete does not fire at all for me, even though the query works :( Am I being dense here?

const { loading, error } = useQuery(
  gql`...`,
  {
    onComplete: data => {
      // transform data.. but never called :/ ???
    },
  }
);

[riccoski]

onComplete does not fire at all for me, even though the query works :( Am I being dense here?

const { loading, error } = useQuery(
  gql`...`,
  {
    onComplete: data => {
      // transform data.. but never called :/ ???
    },
  }
);

It's onCompleted

[hamxabaig]

@jasonpaulos here is the sandbox link where you can reproduce it https://codesandbox.io/s/pensive-worker-3py6j

Initially the query will log fired in console as onCompleted will be called. But when the query is unmounted and then mounted again, its not called anymore but the data retrieved from the cache.

react-apollo: v2.5.8

[hamxabaig]

I think it might be because of this, since neither the query nor the variables have updated, so it returns. Not sure since i'm not that aware of the codebase

https://github.com/apollographql/react-apollo/blob/9e4b573a5ae8ee068bc6f9229e6d84b36974da6b/packages/hooks/src/data/QueryData.ts#L340-L347

[jasonwr]

@vititu @yantakus . I use a different workaround. You have to keep reference to completed query result with react state or instance variable. Make sure you only call setState only if current query result is not equal with previous keep result. Here is example.

<Query
onCompleted={data => {
          if ( this.lastFatchedData !== data) {
            setState({..});
          }
          this.lastFatchedData = data;
}
/>

This isn't a feasible approach. What if the lastFatchedData (you probably meant to say lastFetchedData) is a list (array) of objects and the data is a list of objects each 10 in length but say one of those objects has different variables. This is a shallow comparison. It's not related to this defect but this is not a feasible approach.

[jasonwr]

I think it might be because of this, since neither the query nor the variables have updated, so it returns. Not sure since i'm not that aware of the codebase

https://github.com/apollographql/react-apollo/blob/9e4b573a5ae8ee068bc6f9229e6d84b36974da6b/packages/hooks/src/data/QueryData.ts#L340-L347

Yes but I'm getting the same exact buggy behavior with the fetchPolicy set to no-cache This is a bug.

[hwillson]

@jasonwr If this is still happening with React Apollo 3.0.1, would you mind opening a new issue about this (ideally with a small runnable reproduction)? Thanks!

[jasonwr]

@jasonwr If this is still happening with React Apollo 3.0.1, would you mind opening a new issue about this (ideally with a small runnable reproduction)? Thanks!

@hwillson turns out this might be a user error. I was on 2.5. Just submitted my code to review but the review is going to have to wait for some other things to get through the pipeline. I'll keep you all posted. Thanks.

[websash]

@hwillson The onCompleted callback is still not working correctly in @apollo/react-hooks 3.0.1 It runs on every component rerender when I change its state. Not expected behavior for this callback. I've created a codesandbox https://codesandbox.io/s/jolly-easley-ekm0j
I can open a new issue if there's not one open already

[cristiangrojas]

I can't really believe this is still happening and there's no fix.

[dylanwulf]

@websash Check out #3353. That problem has been fixed, but hasn't been released yet. The fix will be included in the next release

[tresdev]

Is there any other option I can use instead of onCompleted, some funciton I can use after the query result is retrieved whether is done on network or from cache?

[fnune]

Included here too https://github.com/apollographql/react-apollo/issues/3535 is the related issue that Query.onCompleted only fires for the initial fetch if using pollInterval. All subsequent polling requests fail to call Query.onCompleted.

[Algram]

@tresdev we solved it by using the useEffect react hook to react to a completed query:

const { data, loading, error } = useQuery(queryName, {
  pollInterval: 15000,
})

useEffect(() => {
  if (loading || error) {
    return
  }

  // Do your thing with the data here
}, [data, error, loading])

[limekiln]

@Algram this does not work for me since none of the variables are changing on the second query (data, loading, error, networkStatus). Is there another workaround?

[fgiarritiello]

Please, any chance we can get this fixed?

[SamiUrias]

It still not working, why this issue gets closed?

[martdavidson]

This is still happening, can we get this reopened?

[dqunbp]

This is still happening, can we get this reopened?

@hwillson I acknowledge this issue.
Using cache-and-network leads to overuse of the network.
Maybe we need an additional callback, for example onCompletedFromCache ?

[dryoma]

Hi @dqunbp

Maybe we need an additional callback, for example onCompletedFromCache?

I'd suggest having one callback for all "completed" events, but pass some argument with the info on how exactly the data has been fetched: from cache or via network.

[Darklaki]

I'm facing the same issue.
Having one callback and passing an argument whether to fire onCompleted twice (when we get data form cache and then when we receive data from server) would work like a charm.

[PavanBahuguni]

use notifyOnNetworkChange when u are using default fetch-policy (when u havent specified anythig). @Darklaki

[palerdot]

This issue is a dealbreaker and kind of makes the default fetchPolicy: cache-first useless in useQuery ... With this bug it is impossible to prevent subsequent expensive api calls, since the UI is not notified of data from cache ...

Please reopen this issue as it is a significant bug forcing UI to make graphql calls everytime, even though UI could do fine with results from few seconds back for the same set of input.

[PavanBahuguni]

@palerdot I have beein using notifyOnNetworkChange on Query render prop, it does call the onComleted when that data is fetched from network or cache, u dont have to change you'r network policy.

[palerdot]

@PavanBahuguni It does not seem to work for me ... plus I'm using useQuery and it is not working for useQuery hook

[mvirbicianskas]

I can confirm, this is not working for me too in @apollo/[email protected]
Edit:
Weirdly it only doesn't work when you're trying to "fetch" same data from cache consecutively:

1. Fetch data from network - onComplete triggered
2. Try fetch same data from network - onComplete not triggered
   Otherwise it works with:
3. Fetch data from network onComplete triggered
4. Fetch other data from network onComplete triggered
5. Fetch 1. data from cache - onComplete triggered

[himat]

Same as @mvirbicianskas If I try to fetch the same data consecutively, it doesn't work

[dominik-myszkowski]

fetchPolicy: 'network' option fixes it successfully. onCompleted fires every time

[fnune]

fetchPolicy: 'network' option fixes it successfully. onCompleted fires every time

It doesn't fix it: it circumvents the issue by not using the cache. This is about onCompleted not firing when the data comes from the cache.

[Bolza]

It has been 2 years this issues hasn't been fixed and the issue has been closed.
It seems a pretty important bug, it's easy to reproduce and there's a very high chance to end up in this situation. Can anyone explain why it's closed?

[cyberwombat]

Reopened here https://github.com/apollographql/react-apollo/issues/3968 - hopefully it will get some visibility. Pretty much a show stopper for me.

[1awaleed]

I can't believe this is still not fixed.

[farid-ouachrar]

Any update regarding this issue?

[vitrukroman]

fetch policy cache-and-network is not correct overcome even
Imagine you have loading spinner which hides onCompleted .
When data is cached, it is immediately returned, however spinner will be loading until network request is in progress.

I agree that onCompleted event must be called when data is retrieved from cache

[fadomire]

almost 2 years and no explanation of the why it is like that or a fix
seems like it is time to switch from apollo