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.4k stars 2.66k forks source link

Still no alternative to onCompleted change in 3.8 #12056

Open flashtheman opened 3 months ago

flashtheman commented 3 months ago

Issue Description

In Apollo Client 3.8, the behavior changed such that onCompleted is no longer called when another component triggers a refetch of the corresponding query. In versions <= 3.7.x, onCompleted was always invoked when notifyOnNetworkStatusChange was set to true.

We have now been waiting for a year for an alternative to the old behavior, as our app heavily relies on this functionality.

Is there any plan to provide an alternative, or is there an established workaround? The only solution we know of is using useEffect to watch the data object, but this would require significant changes to our application. Because of that, we've been hoping for an official solution to this issue.

Link to Reproduction

none

Reproduction Steps

No response

@apollo/client version

3.8.x

jerelmiller commented 3 months ago

Hey @flashtheman 👋

To clarify, are you saying that after upgrading to > 3.8, notifyOnNetworkStatusChange: true is not triggering the onCompleted callback? If so, that is a bug. A runnable reproduction would be helpful to understand what's happening here if you can provide one.


As for the bad news. At this time we aren't planning to introduce anymore changes to onCompleted or introduce an alternative callback in its place. That change in 3.8 ended up being way more spicy than we had anticipated and is one of those changes we wished we would have waited for a major version to introduce. This is unfortunately one of those APIs that is impossible to make everyone happy. Everyone has a different idea of how it should work so while we fixed the behavior in 3.8 for one camp of individuals, others used it as a feature and saw the change as an introduction of a bug.

Would you be able to provide some context for how you're using onCompleted? Perhaps we can give some recommendations on good/bad practices.

I'm sorry I don't have a better answer for you at this time otherwise.

flashtheman commented 3 months ago

Hi @jerelmiller,

Thank you for your detailed response.

I'll look into creating a runnable reproduction if that would help.

Here’s my specific situation:

I realize you may have thought I was referring to the refetch method returned by useQuery, but I was actually referring to the refetchQueries option from a separate mutation.

jerelmiller commented 3 months ago

That helps! Appreciate the context!

This is one of those cases where it sort of worked by accident in 3.7.x or before. Using that object syntax with refetchQueries actually creates an entirely new ObservableQuery instance under the hood (the thing returned by client.watchQuery(...) which useQuery calls under the hood), so these queries weren't actually connected in any meaningful way. The onCompleted callback was triggered purely because the refetch from the other ObservableQuery instance caused a cache update. As you know, cache updates in 3.7 call onCompleted.

The notifyOnNetworkStatusChange option to refetchQueries here doesn't actually do much for you because you don't actually have access to the ObservableQuery instance that was created using this syntax. I can almost guarantee that removing that notifyOnNetworkStatusChange option using 3.7 or less won't change the behavior you're seeing/expecting with onCompleted. So with that, don't worry about a runnable reproduction since this isn't actually a bug.

That said, I'm curious, what kind of logic are you executing in onCompleted? Perhaps I can provide some help with that logic?

flashtheman commented 2 months ago

We primarily use the onCompleted function to update a state variable. This state variable is either used to render something in the current component or passed down as a prop to a child component. That’s the main use case.

However, starting from version 3.8.x, the state variable no longer updates as expected (because onCompleted is not executed anymore), which, based on your explanation, makes sense.

From what I understand, a possible solution is to directly use the data object from the useQuery hook and then update the state variable inside a useEffect hook that watches for changes in the data object.

jerelmiller commented 2 months ago

Is the state variable you're setting based on the data returned from the query?

If so, we'd recommend using a derived value instead:

// Don't do this:
const [count, setCount] = useState(0);

const { data } = useQuery(query, {
  onCompleted: (data) => setCount(data.items.length)
});

// instead do this:
const { data } = useQuery(query);

const count = data.items.length;

I don't know if this is how you're using it, but if so, use a derived value which will be much more accurate since it will be kept in sync with data as cache writes happen.

emadabdulrahim commented 5 days ago

@jerelmiller Hi, where can I read about the changes made to onCompleted in 3.8? Docs have barely any mention of onCompleted.

For example, does onCompleted fire when the query get value from cache? if not, where is this information in the docs?