apollographql / apollo-client

:rocket:  A fully-featured, production ready caching GraphQL client for every UI framework and GraphQL server.
https://apollographql.com/client
MIT License
19.37k stars 2.66k forks source link

UseLazyQuery does not trigger onCompleted if result remains the same #9338

Open Aloune92 opened 2 years ago

Aloune92 commented 2 years ago

Intended outcome:

I want to query manually using useLazyQuery hook and get the result in the onCompleted callback function. I also use a no-cache fetch policy.

Actual outcome:

The onCompleted callback function is only triggered the first time, if the result of the query has changed. After the first call, I can see in the Network panel of my browser that the query is being executed everytime I click on the button but no further event is being fired. It would be logical that the onCompleted callback function triggers no matter what the fetch policy is.

How to reproduce the issue:

function TestComponent() {
  const [execQuery] = useLazyQuery(MY_QUERY, {
    fetchPolicy: 'no-cache',
    onCompleted: result => {
      console.log('execQuery has completed'); // only fires the first time
    },
  });

  return (
    <>
      <button onClick={() => execQuery()}>click me</button>
    </>
  );
}

Versions

apollo-client : 3.5.7 react : 17.0.2

byanes commented 2 years ago

I am also facing this issue.

maciejregula commented 2 years ago

Apollo client library which has over 2m downloads per week doesn't have any docs regarding how onCompleted behaves. It works totally randomly depending on fetch-policy and apollo/client version. This subject is not new....waiting for over 2 years and still no solution/no docs.

enheit commented 2 years ago

I have the same issue. The onCompleted hook is not triggered after sending query with same variables.

const [lazyQuery] = useLazyQuery(QUERY, { onCompleted: () => {...} })`

lazyQuery({ vairable: 1 }) // response received, onCompleted was triggered
lazyQuery({ vairable: 1 }) // response received, onCompleted was NOT triggered !?!?
DDSAO commented 2 years ago

fetchPolicy: 'cache-and-network' works for me.

MattL75 commented 2 years ago

Facing the same issue. Can also confirm that the fetch policy cache-and-network does trigger onCompleted whereas network-only and no-cache do not trigger it. This was tested with latest beta (3.6.0-beta.11).

Ironically the original reason we were using network-only was to get onCompleted to fire. I'm open to switching to cache-and-network but I'm a bit worried that this scenario will happen again 😐

zacharymorel commented 2 years ago

I am also seeing this same issue. It was introduced when we upgraded our apollo/client package recently from 3.4.17 to 3.5.10.

benjamn commented 2 years ago

We changed a number of things about how useQuery and useLazyQuery are implemented internally in Apollo Client v3.6, so I would recommend attempting to update with npm i @apollo/client@latest when you have a chance!

For example, useLazyQuery is now implemented (more fully) in terms of useQuery, so it should now (with any luck) inherit the same onCompleted and onError calling behavior as useQuery. Even if that behavior is undesirable, this means we can adjust/fix both hooks simultaneously, going forward.

jakequade commented 2 years ago

@benjamn It becomes a question of whether it's a good design choice to have onComplete only fire the first time. Keen on thoughts. Thanks for the explainer on the recent changes.

ice1080 commented 1 year ago

We just updated to 3.7.2 and saw this working with fetchPolicy of network-only. Specifically, I mean I now see onCompleted getting called every time.

tscavnicar commented 1 year ago

Hi, you need to use notifyOnNetworkStatusChange: true. Can confirm that notifyOnNetworkStatusChange solves the problem.

jmtimko5 commented 1 year ago

This is pretty brutal bug honestly. It's completely unexpected behavior that has led to several bugs leaking into production in our product. Can someone on the Apollo side investigate and confirm it is not present on the latest version. I do see a pattern on a lot of these Apollo bugs where there is no attention for months, a new version gets released, then maintainers say please upgrade to the latest version. There is no investigation whether the bug is still present in the new version. Often people confirm the latest version still has the bug, and then the bug remains open or worse it is preemptively closed.

Berger92 commented 1 year ago

@jmtimko5 I don't think this is a bug, it is just the default behavior. It can be a bit annoying that the default behaviour isn't network-only, but it is not a bug 🤷

By default, the useQuery hook checks the Apollo Client cache to see if all the data you requested is already available locally. If all data is available locally, useQuery returns that data and doesn't query your GraphQL server. This cache-first policy is Apollo Client's default fetch policy.

jmtimko5 commented 1 year ago

@Berger92 It certainly seems like a bug. If you look at the post, it is discussing a situation where we have specified the fetchPolicy as no-cache. This policy does not check the cache, nor does it update the cache with the response. I don't think documentation you are quoting applies in this case. We are specifically discussing this case where the developer has specified fetchPolicy: no-cache.

Berger92 commented 1 year ago

@jmtimko5 I see your point, I missed that bit. I just read the docs on no-cache and it is an interesting option because if you use the same query somewhere else without no-cache, I think that's where it can lead to undesirable results, but I am still not convinced that this is a bug, just a really really annoying behavior. Something I didn't expect myself and led me to google my problem and ended up here 😆

mike-tang commented 1 year ago

I ran into a similar issue while trying to trigger a lazy query in an onBlur input event. My onBlur console log was running, but not the lazy query function right after it.

I resolved my issue by specifying the fetchPolicy in the query function, rather than passing it in the useLazyQuery options.

Example:

const TestComponent = () => {
  const [runQuery] = useLazyQuery(MY_QUERY)

  return (
    <Input 
      onBlur={() => {
        console.log('blur')
        runQuery({ 
          fetchPolicy: 'no-cache',
          onCompleted: result => {
            console.log('runQuery has completed')
          }
        }
      }}
    />
  )
}

Passing fetchPolicy into the query function worked for no-cache and seemed to work for other options as well (I'm using cache-first).

Versions: "@apollo/client": "^3.7.8" "react": "18.2.0" "next": "13.1.6"

arel commented 1 year ago

It's surprising that apollo-client still hasn't:

  1. properly documented the behavior of onCompleted
  2. fixed querying. Isn't that its main thing?

https://giphy.com/gifs/abcnetwork-modern-family-phil-dunphy-l3q2NboZyZSrDTM7S

arel commented 1 year ago

As a workaround, you can try adding an arbitrary extra variable to your query!

  const [getUsers, { loading, data }] = useLazyQuery(USERS_QUERY, {
    variables: { anExtraArbitraryVariable: Math.random() },  // with this, onCompleted() gets called (even when cached)
    onCompleted: (response) => {
      setUserList(response.users.data);
    }
  });

UPDATE: this approach works on the reproduced issue here https://github.com/apollographql/apollo-client/issues/7503#issue-771680350, but it didn't work in general for me, so your mileage may vary.

arel commented 1 year ago

Another workaround may be to not rely on onCompleted and use the promise's then() instead:

  const [getUsers, { loading, data }] = useLazyQuery(USERS_QUERY);
  // ...
  getUsers().then((response) => {
    setUserList(response.data.users.data);
  });

(Also, not sure when lazyQuery started returning a promise. It doesn't appear to be in v3.3, but is in v3.7.)

JonScottLee commented 1 year ago

Another workaround may be to not rely on onCompleted and use the promise's then() instead:

  const [getUsers, { loading, data }] = useLazyQuery(USERS_QUERY);
  // ...
  getUsers().then((response) => {
    setUserList(response.data.users.data);
  });

(Also, not sure when lazyQuery started returning a promise. It doesn't appear to be in v3.3, but is in v3.7.)

This is what I ended up doing to resolve the issue. When in doubt, rely on the underlying JS API, if there's not some huge tradeoff in readability. I don't know how onCompleted works. I DO know how Promises work.

coder-Rit commented 9 months ago

In my case, I applied a query to the child component of react which gets rendered multiple times using array.map(), and the query could fetch the data. but the useLazyQuery hook cannot call the onComplete ( probably due to multiple renders, it got confused. ) As I moved the query to the parent component it immediately started working. 👍

sebavuye commented 9 months ago

Another workaround may be to not rely on onCompleted and use the promise's then() instead:

  const [getUsers, { loading, data }] = useLazyQuery(USERS_QUERY);
  // ...
  getUsers().then((response) => {
    setUserList(response.data.users.data);
  });

(Also, not sure when lazyQuery started returning a promise. It doesn't appear to be in v3.3, but is in v3.7.)

The onComplete callback didn't always fire when triggering calls quickly after one another, even though all the calls were executed in the network tab. The promise approach also works for me.

jmtimko5 commented 8 months ago

Ran into this again in a customer demo, just a bummer. 1 year to the day where this caused a production fire previously.

snovos commented 6 months ago

@Aloune92 did you try passing notifyOnNetworkStatusChange: true? It should trigger onComplete on every call.

geoyws commented 6 months ago

notifyOnNetworkStatusChange: true

This doesn't work for me 😞 I had to resort to @arel 's workaround with .then for my hook to reliably run. Cost my team almost a whole day there with debugging.

jmtimko5 commented 2 months ago

This bug is still present on 3.11.4

jmtimko5 commented 2 months ago

@hwillson Really thankful for all the work that you do on this library. I'd be incredibly grateful if someone from the maintainers could address this. We keep running into production incidents because devs on our team use useLazyQuery as documented and then this fails. We have lots of data intensive calls where we are not caching that results. It's very difficult to catch every time someone checks in code that triggers this bug, because the code looks correct.

If there's anything we can do, whether it be financial support or fixing this bug ourselves with some guidance from the maintainers to help speed this along, please let me know.

phryneas commented 2 months ago

@jmtimko5 The problem is that generally, onCompleted is interpreted very differently by different people:

The current behaviour of "onCompleted only triggers if a query result changed" is probably the one we will stick to for now (we have moved more in the direction of making it the general behaviour with #10229), and honestly, given this confusion, and the fact that other libraries are moving away from these kinds of functions (React Query removed onSuccess because of similar confusion), I would not be surprised if we would deprecate these functions at some point in the future.

My recommendation here would be to avoid this callback due to it's ambiguity, and instead use the promise returned from calling the excecution function like it was recommended in this comment.