Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Still no alternative to onCompleted change in 3.8 #12056

Open
flashtheman opened this issue Sep 4, 2024 · 5 comments
Open

Still no alternative to onCompleted change in 3.8 #12056

flashtheman opened this issue Sep 4, 2024 · 5 comments

Comments

@flashtheman
Copy link

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
Copy link
Member

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
Copy link
Author

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 have a central query definition (QUERY) and a central mutation definition (MUTATION).
  • In one part of my application, I use useQuery to execute QUERY, with notifyOnNetworkStatusChange enabled.
  • In another part, I use useMutation to execute MUTATION. This mutation utilizes the refetchQueries array to refetch QUERY. I'm using the same variables for the refetch and even tried adding notifyOnNetworkStatusChange to it like this: refetchQueries: [{ query: QUERY, notifyOnNetworkStatusChange: true, variables: { ... } }].
  • In version 3.7.x, the onCompleted callback of useQuery was triggered when the refetch occurred. However, in version 3.8.x, the onCompleted function is no longer being executed.

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
Copy link
Member

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
Copy link
Author

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants