More detailed explanation of `onCompleted` in the docs

[bignimbus]

As of 3.8.0 we fixed some bugs affecting how and when onCompleted functions were called. onCompleted isn't mentioned very much in our docs so let's plan to add some more details, including:

• How to use with notifyOnNetworkStatusChange
• A note on the future of callbacks in Apollo Client

[flippidippi]

If onCompleted was "fixed" by not allowing other same queries to be notified, I think that is still a very useful use-case. Should there be a different callback to support this? Like onAnyCompleted/onDataUpdated or something?

From my comment in the other thread

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.

[epmatsw]

It's not clear to me how onCompleted can be behaving as intended in its current state. The docs define onCompleted as:

A callback function that's called when your query successfully completes with zero errors

Why would the query completing after refetch or a poll not be considered successfully completing?

[epmatsw]

Perhaps "completing" is just too generic of a term? But it feels very strange that you can have:

• User calls refetch
• Apollo puts a request on the wire
• New data is returned from the hook
• That doesn't count as completing

[flippidippi]

I agree. I want a onDataUpdated hook so I don't have to use a useEffect, which is how we are using onCompleted.

[jensbodal]

that or onRefetch would seem to make sense. Are we supposed to use useEffect for this case or is there a different pattern to follow?

[flippidippi]

I do like onRefetch, makes sense. I'm just not updating for now lol. But it seems like you would have to use a useEffect, which feels bad.

[jerelmiller]

@jensbodal @flippidippi curious what you're looking to do in the callback function? I'm trying to gather some feedback for use cases out there. I'm also curious on the sentiment behind useEffect here. Anything you're willing to share would be helpful. Thanks!

[flippidippi]

In make example from above, where one query is polling and the other is not. I want the non-polling query to be notified if it's data gets updated by the polling query. Inside the non-polling "on data changed" handler (currently onCompleted cause we haven't updated), I use this to refetch related data and reset filter for that component.

[jerelmiller]

@flippidippi apologies for being a bit slow, but I'm not sure I'm following. Here is what I envision what you're saying.

const GET_DATA = gql``
const ANOTHER_QUERY = gql``

function PollingComponent() {
  useQuery(GET_DATA, { pollInterval: 1000 });
}

function NonPollingComponent() {
  const [filter, setFilter] = useState();

  const { refetch } = useQuery(ANOTHER_QUERY)

  useQuery(GET_DATA, { 
    onCompleted: () => {
      setFilter(null);
      refetch()
    }
  });
}

Does this look correct? If so, I'm curious how the filter and refetch plays into this. I'm unsure how all of this stuff is related.

[jensbodal]

Not sure if this is an over-simplification but this is essentially my use case:

const { loading, refetch } = useItemsQuery({
  nextFetchPolicy: 'cache-and-network', // this line is necessary when calling refetch in order to trigger onCompleted
  variables: { input: { count: 3 } },
  onCompleted: ({ items }) => {
    // do stuff with items which updates on refetch and on initial query
  },
});

And alternative with useEffect would be

const { loading, refetch, data } = useItemsQuery({
  variables: { input: { count: 3 } },
});

useEffect(() => {
  // do stuff with data which updates on refetch and initial query
}, [data]);

It just seems weird that onCompleted is only called on the initial query now. But maybe the point is that the caching policy should change if you want it to be called on refetch too?

[jerelmiller]

@jensbodal thanks so much for providing that example! If I may ask, what do you do with items inside that onCompleted function? Are you doing some kind of data manipulation with that value?

[jensbodal]

I can post an example tomorrow but basically mapping/reducing what comes back into an object that we use with useState

[flippidippi]

@jerelmiller

That's basically it. We are wanting to reset state in another component that when the data changes.

Our specific example if for notifications. The notification bubble in the header is polled on an interval. When the notification count changes, we want to indicate in the notification drop down to refetch notifications because the count changed.

Something like this

const GET_COUNT = gql``
const GET_LIST = gql``

function PollingComponent() {
  useQuery(GET_COUNT, { pollInterval: 1000 });
}

function NonPollingComponent() {
  const [importantOnly, setImportantOnly] = useState(false)

  const [getList, list] = useLazyQuery(GET_LIST, {
    variables: { importantOnly }
  })

  useQuery(GET_COUNT, { 
    skip: !opened,
    onCompleted: () => {
      setImportantOnly(false)
      getList({ importantOnly: false })
    }
  });
}

[jerelmiller]

@flippidippi @jensbodal thanks both for your responses! This is super helpful for me to get an idea of what you're doing!

To be totally transparent with you, I'm asking these questions because we are going to start encouraging devs to avoid both the onCompleted and onError callbacks on useQuery (useMutation won't be affected) when possible. Most of the usages we've seen with these callbacks are what we'd consider anti-patterns these days, and we want to encourage better practices with Apollo Client. Dominik from React Query wrote a blog post that provides really good insight into what we're thinking that also applies to Apollo Client.

The behavior we changed for 3.8 is unlikely to be reverted at this point. Unfortunately this API has become pretty controversial as some view the behavior in 3.7 as the correct behavior for this callback and others view 3.8 as the correct behavior. No matter which direction we choose, someone will be unhappy. We definitely realize the knowledge gap and ambiguity in our docs on these APIs and understand that we could have been much more clear on the intended behavior to avoid confusion.

With that said, I'd like to suggest an alternative for both of you to explore that would let you avoid onComplete altogether. Do let me know if these work for you since it would hopefully unblock you from upgrading to 3.8 (if you're wanting to do so).

---

@jensbodal I'd highly encourage you to avoid doing data manipulation and syncing with useState in this callback. You'll cause additional rerenders in your app with the call to your state setter (which may or may not impact perf depending on your app), but more importantly, you're much more prone to getting out-of-sync with the underlying data. This is especially prone to happen if you make direct cache updates to this data. onCompleted was not originally meant to fire when cache updates were made (the behavior that was changed in 3.8), which means that relying on onCompleted for that data manipulation could be error prone. You're bound to miss out on an update, which means you're now showing some stale data somewhere. Check out this section in Dominik's blog for a good illustration of this as it also pertains to Apollo Client.

useQuery already handles keeping data in sync with the cache, regardless of how it was updated. By using an additional useState, you're now duplicating that state in 2 places which means you need to write extra code to keep it in sync.

Instead, we recommend you derive that state and perform the data manipulation in render.

const { data } = useQuery(QUERY)

// Always stays in sync whenever `data` changes
const formattedItems = formatItems(data.items)

// or useMemo if you need a stable object reference
const formattedItems = useMemo(() => formatItems(data.items), [data])

// use the derived state in your app
return formattedItems.map(item => ...)

Please try this out and let me know if you have any troubles moving this to derived state.

---

@flippidippi yours is a very interesting one. I want to point to one key thing you said here (emphasis mine):

We are wanting to reset state in another component that when the data changes.

You specifically mention here that you want to react to data changes. In 3.7, onCompleted would fire at times even with no data changes, so there is a chance you might actually be fetching unnecessarily.

I want to give you another idea though that wouldn't involve the use of onCompleted. I don't know the total picture of your app, so forgive me if there are details I miss, but based on your sample code, I think this would work well.

I'd like for you to take a look at our useFragment hook. This hook was designed to be able to "query" the cache for specific data, then rerender when that changes. By combining this with useEffect, I think you'll get a more consistent behavior.

Since you're on 3.7, you'll need to import this hook with the _experimental suffix to try it out, but please let me know if this works for you. NOTE: you do not have to use this fragment in the query itself.

const FRAGMENT = gql``

function NonPollingComponent() {
  const { data } = useFragment_experimental({ FRAGMENT, from: {...} })
  // I'm omitting variables here since the variables passed to `getList` override these
  const [getList, list] = useLazyQuery(GET_LIST)
  
  useEffect(() => {
    getList({ variables: { importantOnly: false } })
  }, [data.count])
}

Don't worry about the _experimental suffix here. The behavior between 3.7 and 3.8 is pretty much unchanged with the exception of some better TypeScript types in 3.8.

Please try this out and let me know if this is a suitable replacement for onCompleted, which lets you avoid that 2nd useQuery altogether as well.

[dylanwulf]

In our app, we make extensive use of onCompleted for polling queries. For example, when taking payments at physical payment terminals, we will fire a mutation which immediately returns with a "payment task id" and activates the payment terminal. We can then poll the status of the payment using this payment task id. In onCompleted we check the polling result to see if the payment has succeeded yet. Once the payment succeeds, we navigate the user to the Order Confirmation page. Without onCompleted we would have to use useEffect to detect this change & navigate the user, and React seems to recommend using events over effects where feasible (although that article only seems to show user events in its examples, and onCompleted is a network event which is not discussed in that article, so I suppose it could be argued either way)

[jerelmiller]

@dylanwulf appreciate the input here! Out of curiosity, is there any disadvantages to using useEffect in this situation? Or have you found the changes between 3.7 and 3.8 for onComplete difficult to use for your use case?

On the topic of useEffect theory, I think useEffect is fine here. Looking at the useEffect docs, it states:

useEffect is a React Hook that lets you synchronize a component with an external system.

I think you can make the case that your "external system" is the URL, so navigating within useEffect is fine. Certainly does leave it up to interpretation a bit though haha.

I think the point about events over effects is to avoid situations like this:

  // 🔴 Avoid: Event-specific logic inside an Effect
  useEffect(() => {
    if (product.isInCart) {
      showNotification(`Added ${product.name} to the shopping cart!`);
    }
  }, [product]);

  function handleBuyClick() {
    addToCart(product);
  }

  function handleCheckoutClick() {
    addToCart(product);
    navigateTo('/checkout');
  }

Here its better to just call showNotification in the event handlers, rather than waiting for and relying on the effect to trigger it

function buyProduct() {
  addToCart(product);
  showNotification(`Added ${product.name} to the shopping cart!`);
}

As you said, I suppose it could be argued either way 🤷‍♂️

[flippidippi]

@jerelmiller I think I'm on the same lines of thinking as @dylanwulf in that we try to stay away from using useEffect where we can, and the old onCompleted helped us do that. We could make this work with useEffect, but it was much easier to read and avoided an extra useEffect being able to send the query a callback to call when data changed.

We could also fix this issue by managing the state at some higher level and passing it down where needed, but I think that's reverting back to a sort of redux view of state management, where we really like to use Apollo for this.

I understand that there was confusion around the onCompleted and either way would cause issues for some people. Which is why we are thinking having the option for both would be a win win.

[dylanwulf]

Adding more details to my payment example, I think I might have found a situation where useEffect wouldn't work (or at least a situation where I don't trust useEffect to work correctly)

We allow our users to apply multiple partial payments if they want. We don't navigate them to the confirmation page until the balance due hits 0. Each partial payment has its own unique "payment task id". After a partial payment is completed, I need to update the sales order's balance in the apollo client cache (we could refetch, but doing the full calculation is an expensive operation on the backend. much quicker to update the cache). The payment polling result does not return any details about the sales order, not even the order id. The only thing returned from the payment polling is the amount approved by the credit card company. To update the apollo client cache I need to subtract the amount approved from the currently cached balance. I don't think I can do that in a useEffect because react 18 makes useEffects run multiple times -- there is no guarantee that the useEffect will run exactly once when a dependency changes. I can't subtract from the cached totals multiple times because I would end up with the wrong value in the cache. onCompleted provides me with that guarantee that I will only be adjusting the cache once per payment.

I suppose this whole payment process could be thought of as a very-long-mutation instead of mutation-then-polling-query, because we essentially just want to mutate the data one time and get the result one time. But it's not possible to use useMutation for polling queries, so I have to use useQuery. If useQuery's onCompleted callback gets deprecated, we could always build something custom using client.query() so at least there's still a solution no matter what

[phryneas]

@dylanwulf to be honest, this seems at odds with what @flippidippi pointed out in https://github.com/apollographql/apollo-client/issues/11306#issuecomment-1840850630 :

onCompleted can fire in multiple components, no matter if they initiated this or not. So if your component would mount twice, onCompleted would fire twice, doing some global data mutation twice.

To be honest, this is the general problem with onCompleted: there are too many interpretations of when it should run:

• data is already in the cache when the component is mounted
• another component initiated the same query and it finishes
• only a query initiated by this component finishes

Every user will have different expectations, and as soon as we fix one user's expectations, we break another user's.

This might call for some kind of cache-level callback (similar to typePolicies) instead of a component/hook-level configuration, as that would have clearer semantics.

[dylanwulf]

onCompleted can fire in multiple components, no matter if they initiated this or not. So if your component would mount twice, onCompleted would fire twice, doing some global data mutation twice.

That's true, but if I can guarantee that my component is only mounted once then I can guarantee that onCompleted will only run once. Whereas with useEffect I can't make any guarantees about the number of times it will run. I just don't think useEffect is a suitable alternative for my use case.

This might call for some kind of cache-level callback (similar to typePolicies) instead of a component/hook-level configuration, as that would have clearer semantics.

Unfortunately the payment result returned from our backend has no linkage to the sales order whatsoever. So the only place I know which sales order to update is at the component/hook-level

[jensbodal]

@jerelmiller thanks for the feedback, I've refactored now to neither use useEffect or onCompleted and remove the extra state. Instead I just set a value based on data changing which I'm doing with useMemo (which might be overkill haven't validated without it yet).

const { data, loading, refetch } = useItemsQuery({
  nextFetchPolicy: 'cache-and-network', // this line is necessary when calling refetch in order to trigger onCompleted
  variables: { input: { count: 3 } },
});

const itemNameOptions: ItemNameOptions = useMemo(
  () =>
    data?.itemNames.__typename === 'ItemNames'
      ? data.itemNames.displayNames.map((displayName) => ({
          name: displayName,
          anotherProp: getOtherPropValue(displayName),
        }))
      : [],
  [data],
);

[jmclain20]

We are experiencing the same issue here where we are using onCompleted to avoid having multiple useEffects fire as we only need to do the update when a network data call is competed (polling or refetch included). Is there a reason we shouldn't use the notifyOnNetworkStatusChange to ensure that the onCompleted is fired with polling and refetch calls? This seems to be the new behavior to enable the onCompleted with these functions.

[jerelmiller]

Hey all 👋

Appreciated all the discussion we've had in this issue! The information provided has been super helpful.

Unfortunately I fear we are starting to get a bit far away from the initial point of this issue which was to track the progress on providing better documentation for the behavior of onCompleted (totally my fault since I started asking questions 😆). I fear this issue might attract bug reports that might be missed due to the nature of this issue.

Because of this, I'm going to lock this issue to maintainers and ask that you create new issues for bug reports to ensure we can track those correctly. Thanks again for the wonderful discussion!

[jerelmiller]

@jmclain20 to answer your question, totally fine to use notifyOnNetworkStatusChange to ensure onCompleted is fired. This is our recommended approach, so no reason not to use it 🙂