apollo-client: Query Functionality with `onCompleted` is broken in v3.0.0 release (CodeSandbox repro)

@benjamn and @hwillson Thanks for tackling this.

Current State of things

As of the moment of writing, in 3.1.1:

• An errorred query no longer re-renders endlessly 🎉
• A cache-first query with onCompleted and/or onError no longer re-renders endlessly 🎉
• A network-only query no longer pings the server endlessly 🎉

Pending bugs

• The onCompleted callback should always fire whenever a query is resolved (either from network or cache). https://github.com/apollographql/react-apollo/issues/4000

Currently only network-only requests have their onCompleted fired after every query execution. For cache-first ones, the onCompleted fires only once. Unless this is documented somewhere, I would flag this as a bug since our onCompleted logic shouldn’t have to account for the origin of this data, i.e. It should be cache agnostic

• Changing a query variable on a lazy query should not automatically fire it. https://github.com/apollographql/react-apollo/issues/3729

This should definitely happen for useQuery hooks, but not for useLazyQuery hooks (since they wouldn’t be lazy anymore). Again if this is the expected functionality it should ideally be documented

Reproduction of pending bugs

You can observe both these bugs in the following codesandbox --> https://codesandbox.io/s/apollo-reference-equality-op12z?file=/src/App.js

• To reproduce the onCompleted error: Change fetchPolicy to cache-first, click “Run Query” multiple times and see how onCompleted only fires once

• To reproduce the variables with useLazyQuery error: Click “Run Query” once and then start clicking “Change foo”. Clicking “change foo” should not cause a query to run

Additional Context

Out of the two bugs listed above (in the order that they are mentioned):

• The first is within the scope of this ticket and a requirement in order to close it
• The second isn’t necessarily related to this ticket (although mentioned) and I understand that it may require a more complex solution through a modification of your query observables. Thus, I’m willing to port (2) to another issue in order to help close the existing one

Thanks again!

This issue was fixed in https://github.com/apollographql/apollo-client/pull/6588, and will be coming in @apollo/client@3.1.0 very soon.

@brainkim it’s not. Unfortunately, I believe it’s now worse than before, since onCompleted gets only triggered once for all lazy queries with network-only policy, regardless of the number of times they are called. This means that even if I perform multiple network requests, I’ll still have only 1 onCompleted callback fired.

Please refer to this comment https://github.com/apollographql/apollo-client/issues/6636#issuecomment-666310747 and its reproduction instructions to validate the bugs. I was able to validate them in apollo-client@3.5.3.

Should we re-open this?

@3nvi I believe both of the problems demonstrated in your reproduction are related to the fact that useLazyQuery uses client.watchQuery behind the scenes, when it should just be using client.query:

• Refetching a network-only query when a variable changes is expected if the query is being watched, but would not happen if the query only executes when explicitly requested.
• The problem with onCompleted not firing is more loosely related, but I’m sure that’s something we could fix in the process of rewriting useLazyQuery to avoid watching, since we should be able to hook into the Promise returned by client.query.

Although we’re about to release v3.2.0 with the changes that are currently on the release-3.2 branch, we haven’t forgotten about this issue. We hope to include a fix in a 3.2.x (or a 3.3 beta) version soon.

I’ve noticed that this is also happening with a regular useQuery hook when an onCompleted function is specified (even if its a noop) and the fetchPolicy is set to anything except cache-first or cache-only

Would be nice to have a proper onComplete fired on refetch even if it’s from cache.

The most similar behavior I could come up with meanwhile was:

  useEffect(() => {
    if (query.networkStatus === NetworkStatus.ready) {
      ...on complete...
    }
  }, [query.networkStatus])

I just discovered that after upgrading from apollo 2 to 3.1.5 that my useLazyQuery are not always firing the onCompleted. Current workaround for me seems to be changing usage of useLazyQuery to:

client.query({ query: <QUERY> }).then(res =>{ ... } );

This issue was not fixed for three years…

I’m having the same problems with useLazyQuery

Heads up: we’re not quite ready to publish @apollo/client@3.1.0 yet, but I’ve published an @apollo/client@3.1.0-pre.0 release that includes #6588, if you want to try that in the meantime. 🙏

Update: thanks to @3nvi’s reproduction, I can confirm the fix: https://codesandbox.io/s/apollo-reference-equality-t0ttx

As always, I appreciate you’ re juggling a lot of things, so thanks in advance

👍 on this, thanks for the work you’re putting into this project!

I also have an issue with useLazyQuery that I think might relate to this?

I have an ApolloClient instance that I use in my ApolloProvider that I need to reinitialize once in a while because a couple of settings change during the lifetime of the app (mostly HTTP headers and connection params). Therefore, instead of having the client be created outside the scope of my React tree, it’s generated within a custom hook. However, it seems like every time there’s a new value passed to ApolloProvider for the client prop, all queries using useLazyQuery are relaunched automatically – and they seem to be launched with the latest variables they were called with. I would expect that they wouldn’t be launched until there’s another call to the functions provided by the useLazyQuery hooks, right?

Maybe there’s something wrong with the way I’m reloading the client though, and that it shouldn’t go through React updates. I couldn’t find any documentation on it. Let me know!

I’m on 3.1.1 and onCompleted never calls if a useLazyQuery has its result cached.

I don’t think I’ve seen this mentioned yet as it probably comes from the same underlying behavior…but a useLazyQuery with a onCompleted does not get called again if a user specifies a refetchQueries array in a useMutation method call.

Such as

const [updateAlbumReadStatus] = useMutation({
        // this triggers a network refetch but does not call the onComplete() of my UnreadAlbums useLazyQuery
        refetchQueries: ["UnreadAlbums"]
    });

The bug calling onCompleted looks like a long-running holdover from 2.x. https://github.com/apollographql/react-apollo/issues/2177

@apollo/client@3.1.0-pre.0

Thank you so much @benjamn ! It’s working for me, great!