Changes to `context` cause refetches in `useQuery` about apollo-client HOT 12 OPEN

Hey @kjhuang-db 👋

I don't believe that will work since useQuery does a deep equality check to compare context values between render. Since you're assigning the ref in render before useQuery is executed, the next render will see the updated value and trigger the fetch again.

On top of that React's docs state that you shouldn't read or write refs during render unless its used for initialization as it could make your component behavior unpredicatable. See the caveats section in the docs.

I mostly made this note for myself so that I didn't forget about this potential improvement after responding to the thread linked in my message. I and the team think this is fairly surprising behavior in the library and we should address it so that changes to context don't trigger network requests itself. Instead, it should just apply the new context value on the next fetch. This is something our suspense hooks do already, but we should also port that logic over to useQuery.

from apollo-client.

Hey @jerelmiller thanks for the helpful context.

I believe just wrapping the ref change with a useEffect should fix the React render concern.

As of the equality comparison, @kjhuang-db had pointed me earlier that it doesn't do a deep check when the shallow one passes -> https://github.com/benjamn/wryware/blob/main/packages/equality/src/index.ts#L21-L23

which is achieved (in a hack way though) by using the static reference.

What I can't say for sure now, without diving deeper into how context is propagated, is whether the static reference won't cause other issues later on.

Totally agreed and +1 that this can create unpredictable behavior (i.e.: it's not very obvious when looking only at the useQuery interface 😅)

from apollo-client.

Just a note that I had a problem which was causing me real headaches for a while, which seems to be caused by this. I'm very lucky that I decided to join the Discord server and do a search for "abortcontroller" just now and found the thread linked above! I think it probably saved me a lot of debugging time. :)

My situation:

I'm working on a React Native app, which needs to refetch a screen's main query on app foreground (using AppState). After foregrounding my app it's very likely that a user will tap a button that kicks off a mutation (which modifies the data required by the query). The query response is usually received by my app after the mutation's optimisticResponse has been applied, causing the cache to revert to the pre-mutation value, which is a problem for me.

So I need to cancel my query refetch when a mutation is kicked off, so I do that using an AbortController:

handleXButtonPress = () => {
  abortControllerRef.current.abort();
  abortControllerRef.current = new AbortController();
  
  mutation({
    ...
  })
}

However, even though I avoid that race condition by aborting the query on foreground, my mutation's optimisticResponse causes a cache update and therefore rerenders my screen component. This of course, due to the deep equality check, causes my query to refetch while my mutation is in flight, causing the same issue (when the server receives the refetch request, it hasn't usually applied the mutation to the DB).

My solution was to initially convert to useLazyQuery, but I decided instead to go back to useQuery but only pass the AbortController's signal into my refetch calls (not into the useQuery itself). This is OK for me, as my screen doesn't allow mutations to occur until the first query call has completed (so I don't need to abort).

from apollo-client.

bump

from apollo-client.

actually you can use a ref to store the context value (always update it synchronously)

const valRef = useRef(val)
valRef.current = val;
useQuery(query, {
   context: {
     valRef
   }
})

this way you can pass through the dynamic context value without introducing side effect

from apollo-client.

@jerelmiller does it work for you?

from apollo-client.

@gabrieldutra ah you're right. I missed the small detail that valRef was passed directly and not valRef.current in the code example above, so you're right, this would likely circumvent that deep equality check. You're right though that it may or may not cause downstream issues. To my knowledge, we don't really test for that in the codebase, so I'm not entirely sure what the downsides would be. It should more or less be a passthrough to the link chain, but I'd need to double check that.

from apollo-client.

yep ..we also misunderstood the "deep equal", turns out it is not that "deep", it is a normal react style equal check

from apollo-client.

https://github.com/streamich/react-use/blob/master/src/useLatest.ts , a simple and similar idea to wrap your variable as a ref, if you want

from apollo-client.

@jerelmiller I didnt realize you are actually from apollo team member. if so yeah it would be great to address this fine-grained control at apollo level.

For now I will use useRef + write it in render (even react is against it) to bypass the equal check

from apollo-client.

@jerelmiller is there a timeline where apollo might fix it in the react-hook lib?

from apollo-client.

@kjhuang-db we don't have a timeline right now, but I'm willing to bet we'll put this in the next minor version which we'll release in early July. We want to do this in a minor so that we can fix it + provide an option to revert to the old behavior if you rely on this currently.

from apollo-client.