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

useQuery onCompleted callback is one render out of date #12316

Open
DavidA94 opened this issue Jan 28, 2025 · 6 comments · May be fixed by #12319
Open

useQuery onCompleted callback is one render out of date #12316

DavidA94 opened this issue Jan 28, 2025 · 6 comments · May be fixed by #12319
Assignees

Comments

@DavidA94
Copy link

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

phryneas added a commit that referenced this issue Jan 29, 2025
@phryneas
Copy link
Member

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 useLayoutEffect instead, 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.

@phryneas
Copy link
Member

It seems my attempted fix in #12319 works in your replication.

Could you please try it in your app and report back?

npm i https://pkg.pr.new/@apollo/client@12319

@phryneas phryneas self-assigned this Jan 29, 2025
@DavidA94
Copy link
Author

I updated my package.json to 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
Copy link
Member

phryneas commented 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 onCompleted callback - 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
Copy link
Author

DavidA94 commented Jan 30, 2025

I would ultimately recommend you to migrate off the onCompleted callback - we will deprecate that soon, for a lot of reasons I already laid out in my previous comment.

This is ultimately what we've already done, but the workaround I found also required moving off of useQuery to instead a useEffect with a client.query inside of it.

Our actual pattern was:

useQuery(..., {
    onCompleted: (data) => {
        setExternalStore(transformData(data));
    }
);

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 variables don't change between renders, then onCompleted is not called).

What I wouldn't want is

const { data } = useQuery(...)
setExternalStore(transformData(data));

because that will cause extra re-renders every time the hook runs, since the transformation creates a new object.

@phryneas
Copy link
Member

Why not

const { data } = useQuery(...)
useEffect(() => {
  setExternalStore(transformData(data));
}, [data])

synchronizing with external systems is pretty much exactly what useEffect is made for.

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