Changes to `context` cause refetches in `useQuery`

[jerelmiller]

Issue Description

Changes to context can cause new network requests when updated between renders. This is due to the fact that we do a deep equality check on options and if they change, execute a reobserve. In many cases, this works ok as the applied options should just cause the client to read from the cache again, but when used with a fetchPolicy like no-cache, this can cause new refetches. This is especially problematic if you want to use AbortController to handle cancelling previous fetches.

We should also consider expanding this to other options as well. For example, a change to errorPolicy should not cause a refetch but rather should be applied to the next fetch. Suspense hooks already have this behavior built-in.

We should also consider adding the ability to revert the behavior back to its current form where changes to context would cause refetches in case there are users that currently rely on this.

Link to Reproduction

https://discord.com/channels/1022972389463687228/1235943877765238847/1235943877765238847

Reproduction Steps

No response

@apollo/client version

3.10.2

[kjhuang-db]

bump

[kjhuang-db]

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

[kjhuang-db]

@jerelmiller does it work for you?

[jerelmiller]

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.

[gabrieldutra]

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 😅)

[jerelmiller]

@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.

[kjhuang-db]

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

[kjhuang-db]

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

[kjhuang-db]

@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

[kjhuang-db]

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

[jerelmiller]

@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.

[markymc]

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).