useQuery onCompleted callback is one render out of date #12316
Description
Issue Description
I am querying a GraphQL endpoint with dynamic variables. I expect to transform the data inside the onCompleted function, and set a variable based on the dynamic variable. However, the Apollo client is caching the old onCompleted callback, so that it's one out of date, which gives me incorrect data.
To show this, I've written an MRE which console logs inside the onCompleted function. From the parent, I pass a prop down which is printed in the log statement. The prop is a number and each re-render increases it by one. Instead of seeing n in my logs, though, I see n - 1 for everything after the first one.
| Render Count | Prop Value | Expected Log Value | Actual Log Value |
|---|---|---|---|
| Render #1 | 0 | Completed callback: 0 | Completed callback: 0 |
| Render #2 | 1 | Completed callback: 1 | Completed callback: 0 |
| Render #3 | 2 | Completed callback: 2 | Completed callback: 1 |
| Render #4 | 3 | Completed callback: 3 | Completed callback: 2 |
| Render #5 | 4 | Completed callback: 4 | Completed callback: 3 |
MRE code
index.tsx
import React from 'react'
import ReactDOM from 'react-dom/client'
import App from './App'
import { ApolloClient, InMemoryCache, ApolloProvider } from '@apollo/client';
const client = new ApolloClient({
uri: 'https://flyby-router-demo.herokuapp.com/',
cache: new InMemoryCache(),
});
ReactDOM.createRoot(document.getElementById('root') as HTMLElement).render(
<React.StrictMode>
<ApolloProvider client={client}>
<App />
</ApolloProvider>
</React.StrictMode>
)App.tsx
import "./App.css";
import { useQuery, gql } from "@apollo/client";
import { useState } from "react";
const GET_LOCATION = gql`
query GetLocationDetails($locationId: ID!) {
location(id: $locationId) {
id
name
description
photo
overallRating
reviewsForLocation {
id
comment
rating
}
}
}
`;
function DisplayLocations({ lastStateCount }) {
const { loading, error, data } = useQuery(GET_LOCATION, {
variables: {
locationId: "loc-1",
ignore: lastStateCount,
},
onCompleted: (data) => {
console.log("Completed callback:", lastStateCount);
},
});
if (loading) return <p>Loading...</p>;
if (error) return <p>Error : {error.message}</p>;
return [data.location].map(({ id, name, description, photo }) => (
<div key={id}>
<h3>
{name} ({id})
</h3>
<img width="400" height="250" alt="location-reference" src={`${photo}`} />
<br />
<b>About this location:</b>
<p>{description}</p>
<br />
</div>
));
}
export default function App() {
const [state, setState] = useState(0);
return (
<main>
<button onClick={() => setState(state + 1)}>
Click me to increase the counters.
</button>
<p>
<b>Note:</b> The below text is wrong for the first render. Click the
button for the text to match the console logs. The first one works, but
it fails from there on out.
</p>
Expected console log: "Complete callback: <b>{state}</b>"
<br />
What you see: "Complete callback: <b>{state - 1}</b>"
<br />
<br />
<DisplayLocations lastStateCount={state} />
</main>
);
}package.json
"dependencies": {
"@apollo/client": "^3.12.8"
},If it'll load, I've also made a Repl here: https://replit.com/@DavidAntonucci/ApolloBugMRE#src/App.tsx
Link to Reproduction
https://replit.com/@DavidAntonucci/ApolloBugMRE#src/App.tsx
Reproduction Steps
No response
@apollo/client version
3.12.8
Metadata
Metadata
Assignees
Labels
Type
Projects
Milestone
Relationships
Participants
Activity
attempt to alleviate #12316
phryneas commentedon Jan 29, 2025
This seems to only happen for cache hits.
What happens here is that we get the cache hit & start a rerender before the ref for the callback can be updated in a
useEffect.I'm trying this with a
useLayoutEffectinstead, but if that doesn't work I don't think we can do anything - we cannot update these callbacks during render as the render might never commit due to a parallel component suspend and the callbacks would refer to a "ghost render" then.It comes down to: we need to deprecate and remove these callbacks at some point.
They have tons of interpretation problems like "should a cache hit trigger
onCompleted? Should an update of the cache from another component trigger it?" and now this.useQuerywould execute an outdated callback reference. #12319phryneas commentedon Jan 29, 2025
It seems my attempted fix in #12319 works in your replication.
Could you please try it in your app and report back?
DavidA94 commentedon Jan 29, 2025
I updated my
package.jsonto be"@apollo/client": "https://pkg.pr.new/@apollo/client@12319", since Repl, where I'm testing, doesn't allow me to directly run that NPM command.I see the same bug.
phryneas commentedon Jan 30, 2025
That is weird - I did the same and it disappeared for me.
That said, I believe that is all we can do from our side to try and fix this - I'll check in with my colleagues tomorrow, but I don't see a good way forward here - we are bound by React's limitations for this one and breaking those would mean bugs in other cases. React unfortunately invites those "stale reference" situations.
I would ultimately recommend you to migrate off the
onCompletedcallback - we will deprecate that soon, for a lot of reasons I already laid out in my previous comment.Here's also a blog article why React Query removed these callbacks - we share a lot of their concerns by now.
DavidA94 commentedon Jan 30, 2025
This is ultimately what we've already done, but the workaround I found also required moving off of
useQueryto instead auseEffectwith aclient.queryinside of it.Our actual pattern was:
This ensured we only updated our store when it was required, and not every time the hook ran. (Yes, we're dealing with cache hits, but if the
variablesdon't change between renders, thenonCompletedis not called).What I wouldn't want is
because that will cause extra re-renders every time the hook runs, since the transformation creates a new object.
phryneas commentedon Jan 31, 2025
Why not
synchronizing with external systems is pretty much exactly what
useEffectis made for.DavidA94 commentedon Feb 5, 2025
That could possibly work. In my case, I ended up also needing to know if I had a cache hit, which also isn't supported by
useQuery, so I won't be doubling back to try this technique.Overall on the issue, though: is it reasonable to at least ask that the documentation here, and anywhere else applicable, be updated to warn of the pitfalls?
phryneas commentedon Feb 6, 2025
We are in the process of completely deprecating these callbacks - once 3.13 is released those docs will contain a deprecation warning.
useQueryanduseLazyQuerylifecycle hooks #12352