Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

Query.onCompleted isn't fired after fetching data from the cache #2177

Closed
elessarperm opened this issue Jul 12, 2018 · 77 comments · Fixed by #2190
Closed

Query.onCompleted isn't fired after fetching data from the cache #2177

elessarperm opened this issue Jul 12, 2018 · 77 comments · Fixed by #2190
Assignees

Comments

@elessarperm
Copy link

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
Copy link
Contributor

olistic commented Jul 17, 2018

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
Copy link

Dartv commented Aug 9, 2018

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

@serranoarevalo
Copy link

Any workaround for this?

@serranoarevalo
Copy link

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

@hxuanhung
Copy link

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

@serranoarevalo
Copy link

@Dartv
Copy link

Dartv commented Aug 13, 2018

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

@laspluviosillas
Copy link

laspluviosillas commented Aug 21, 2018

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
Copy link

Cabezaa commented Aug 23, 2018

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

@franzejr
Copy link

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

@laspluviosillas
Copy link

laspluviosillas commented Aug 24, 2018

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
Copy link

yantakus commented Sep 3, 2018

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
Copy link
Member

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

@Dartv
Copy link

Dartv commented Sep 28, 2018

onCompleted still isn't fired after mutation automatically updates cache

@hwillson
Copy link
Member

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

@Dartv
Copy link

Dartv commented Sep 28, 2018

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
Copy link

vititu commented Sep 28, 2018

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
Copy link

tab00 commented Oct 4, 2018

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
Copy link

tab00 commented Oct 4, 2018

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
Copy link

riccoski commented Oct 10, 2018

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

@reyesreg
Copy link

reyesreg commented Oct 24, 2018

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
Copy link

SeanningTatum commented Oct 28, 2018

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
Copy link

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

@tresdev
Copy link

tresdev commented Oct 30, 2019

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
Copy link

fnune commented Nov 13, 2019

Included here too #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
Copy link

Algram commented Nov 13, 2019

@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
Copy link

@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
Copy link

Please, any chance we can get this fixed?

@SamiUrias
Copy link

It still not working, why this issue gets closed?

@martdavidson
Copy link

This is still happening, can we get this reopened?

@dqunbp
Copy link
Contributor

dqunbp commented Mar 18, 2020

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
Copy link

dryoma commented Mar 19, 2020

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.

@lukasz-szulborski
Copy link

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
Copy link

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

@palerdot
Copy link

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
Copy link

@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
Copy link

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

@mvirbicianskas
Copy link

mvirbicianskas commented Apr 6, 2020

I can confirm, this is not working for me too in @apollo/react-hooks@3.1.3
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
Copy link

himat commented Apr 11, 2020

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

@dominik-myszkowski
Copy link

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

@fnune
Copy link

fnune commented Apr 30, 2020

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
Copy link

Bolza commented May 13, 2020

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
Copy link

Reopened here #3968 - hopefully it will get some visibility. Pretty much a show stopper for me.

@1awaleed
Copy link

I can't believe this is still not fixed.

@farid-ouachrar
Copy link

Any update regarding this issue?

@vitrukroman
Copy link

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
Copy link

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.