onComplete not being called after doing a refetch

[glenngijsberts]

Issue Description

After an investigation we found out that refetching (using refetch, polling, fetchMore) is not working properly in combination with the onComplete method. On version 3.7.17 it did, in the sense that onComplete was called when a refetch was done. On version 3.8.0 (but also 3.8.1) the onComplete method doesn't get called anymore after a refetch was done. This problem occurs for refetch but also using pollInterval, fetchMore and perhaps other methods that are refetching data.

Link to Reproduction

https://codesandbox.io/p/sandbox/cool-paper-l7wlk2

Reproduction Steps

We've created reproductions in the codesandbox, that shows the problems between the Apollo client versions.

[bignimbus]

Hi @glenngijsberts 👋🏻 thanks for the report! The team will take a look at this as soon as we can, one of us is on vacation and two of us are traveling to a conference. I'm curious as to whether #10229 may have something to do with this, though it's a surface-level observation that will need to be validated.

I can see the issue in your codesandbox but it appears that my fork isn't allowed to query your GraphQL server(?). I'm trying to validate whether adding notifyOnNetworkStatusChange: true to the options passed to useQuery resolves the issue - maybe you could try it on your end?

[glenngijsberts]

@bignimbus thanks for your quick response 💯

I've added the APOLLO_URI const in apolloClient.js, there you need to put the url of your sandbox, then it should work. notifyOnNetworkStatusChange: true does work for now indeed, it's also what we're using in production for the time being.

If this is the way to go, I think it would be nice to clarify this in the docs maybe or mention it somewhere in the release notes 😄

Thanks for your help!

[dylanwulf]

I've noticed this behavior in 3.8 too and I agree it should be mentioned in the docs

[av-k]

@glenngijsberts I've already created a similar ticket:
#11140
It was quickly closed by @alessbell, but at the same time, I saw some mentions of this ticket during fixes in the code.
let's hope that the guys fix something.

[alessbell]

Thanks all - I'm currently on mobile but will take a closer look once I'm at my laptop. We can definitely call this out in the docs 👍

[bignimbus]

Thanks all, looking forward to making these updates. @av-k I appreciate the cross-link of the issue. Some feedback: I’m sure your comment was meant to be helpful but it could be interpreted as calling a maintainer out in a way that I’m hoping you didn’t intend, just wanted to make you aware 🙏🏻

[av-k]

@bignimbus I just wanted to share some context about the already existing context of the known problem with extra details.

A request from my side - please do not rush to close tickets in case even the author has not had time to answer your own question.

[phineasla]

I also noticed that updating variables in useQuery fix the issue. Is this the intended behaviour for 3.8?

By updating the variables, in my case previousResult?.networkStatus will be NetworkStatus.setVariables and result.networkStatus will be NetworkStatus.ready, making the condition previousResult?.networkStatus !== result.networkStatus valid and call onCompleted

// https://rickandmortyapi.com/graphql
const GET_CHARACTERS = gql`
  query Query($page: Int) {
    characters(page: $page) {
      info {
        count
      }
      results {
        name
      }
    }
  }
`;

export function Characters() {
  const [page, setPage] = useState(1);
  const query = useQuery(GET_CHARACTERS, { variables: { page: 1 } }); // Won't call onComplete
  // const query = useQuery(GET_CHARACTERS, { variables: { page } }); // Will call onComplete
  return (
    <>
      <div>
        {query.data?.characters?.results?.map((char: { name: string }) => (
          <div>{char?.name}</div>
        ))}
      </div>
      <button
        onClick={() => {
          query.fetchMore({ variables: { page: page + 1 } });
          setPage(page + 1);
        }}
      >
        Next page
      </button>
      <div> Current page {page}</div>
    </>
  );
}

[jsfeb26]

We have a similar issue after updating to 3.8 but only with a unit test

We have something like this:

  const { data, loading } = useQuery(
    GET_CHARTS,
    {
      variables: {
        id: chartId,
      },
      onCompleted,
      fetchPolicy: 'no-cache',
  })

When running the tests onCompleted is never called but data is populated correctly so I think the mocks and MockProvider are set up correctly.

If I remove the fetchPolicy or even change the fetchPolicy to cache-and-network then onCompleted gets called.

Any idea why this might be happening?

[sflahave]

Can confirm - this is a bug that seems to be introduced with 3.8. Reverting back to 3.7.17 fixes it for us.

[MattFox1388]

Also confirming this is a bug that I've encountered when upgrading to 3.8. Gonna stay with 3.7.

[smyrick]

Hey everyone, I talked with the Apollo Client team and have an update:

From the AC maintainers, this has been one of those issues where one group of people think the old behavior was a bug, but other groups of people think this new behavior is a bug, so we chose a path forward that at least has a solution for both groups, even if that requires code changes.

See this PR for more info on the behavior: #10229

---

Adding notifyOnNetworkStatusChange: true should fix your issue because it causes the networkStatus to change for the refetch. The PR linked above essentially won’t trigger onCompleted if the network status never changes nor does the data.

---

Depending on what kind of logic is in your callback, another option could be to await the promise returned from refetch and have it perform whatever you need when it’s finished

Instead of:

const { refetch } = useQuery(QUERY, {
 onCompleted: (result) => {
  // do something with result
 }
})

Do this:

const { refetch } = useQuery(...)

<button
 onClick={async () => {
  const result = await refetch()
  // do something with result
 }
/>

[av-k]

Hey everyone, I talked with the Apollo Client team and have an update:

From the AC maintainers, this has been one of those issues where one group of people think the old behavior was a bug, but other groups of people think this new behavior is a bug, so we chose a path forward that at least has a solution for both groups, even if that requires code changes.

See this PR for more info on the behavior: #10229

Adding notifyOnNetworkStatusChange: true should fix your issue because it causes the networkStatus to change for the refetch. The PR linked above essentially won’t trigger onCompleted if the network status never changes nor does the data.

Depending on what kind of logic is in your callback, another option could be to await the promise returned from refetch and have it perform whatever you need when it’s finished

Instead of:

const { refetch } = useQuery(QUERY, {
 onCompleted: (result) => {
  // do something with result
 }
})

Do this:

const { refetch } = useQuery(...)

<button
 onClick={async () => {
  const result = await refetch()
  // do something with result
 }
/>

I have already mentioned here in August the linked topic with detailed answers regarding the nature of the error (and mentioned there #10229).

Described by you approach is badly extendable if 2+ components will use one source data it'll be necessary to manage the data state with useState or something like that. And what is the strangest thing is that to use useQuery for goals for which it wasn't created. useQuery usually used for making calls on the component mounting life-circle. Using useQuery for asynchronous logic (user events etc.) as we can see not a good idea. For these goals useQuery can be rewritten by useLazyQuery.

[flippidippi]

We are having the same issue but a little more nuanced. We have a query in a component on the page, say GetItemCount, and in a different part of the page we are using the same exact query. One of those queries is running on polling and the other is not. The one running polling successfully calls onCompleted when the value changes, but the other query does not. Reverting back to 3.7 fixes this issue.

[bignimbus]

Hi all 👋🏻 as mentioned above, the current onCompleted behavior as shown in these runnable reproductions is working as intended. We will update the documentation to add more details about onCompleted so that users can better understand how to use this part of the API. I'm closing this issue in favor of #11306 to track the actionable work we've identified so far.

If you still believe there is a bug please open a new issue with a runnable reproduction that demonstrates the buggy behavior.