Fix accidental double network call with useLazyQuery on some calls of the execute function

[jerelmiller]

Fixes #9448
Fixes #11201

Fixes an issue where useLazyQuery would issue 2 network calls when calling the execute function with no arguments after having called it previously with arguments.

This was mainly due to how the callback function used React refs to store values from the execution function that were passed back to useQuery in the render function. When calling execute without arguments, it would use the previous stored options as the base for merging, which would include variables. Instead, we now properly use options as a dependency to the execute useCallback to ensure that it is kept up-to-date with dependencies. With this, we merge passing options from the hook in the execute function instead of relying on the value from a previous ref to get this.

[changeset-bot]

🦋 Changeset detected

Latest commit: 95ab853

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@apollo/client	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

size-limit report 📦

Path	Size
dist/apollo-client.min.cjs	38.13 KB (-0.01% 🔽)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs"	45.94 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/main.cjs" (production)	43.48 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js"	33.86 KB (0%)
import { ApolloClient, InMemoryCache, HttpLink } from "dist/index.js" (production)	31.79 KB (0%)
import { ApolloProvider } from "dist/react/index.js"	1.23 KB (0%)
import { ApolloProvider } from "dist/react/index.js" (production)	1.22 KB (0%)
import { useQuery } from "dist/react/index.js"	5.2 KB (0%)
import { useQuery } from "dist/react/index.js" (production)	4.27 KB (0%)
import { useLazyQuery } from "dist/react/index.js"	5.5 KB (0%)
import { useLazyQuery } from "dist/react/index.js" (production)	4.58 KB (0%)
import { useMutation } from "dist/react/index.js"	3.51 KB (0%)
import { useMutation } from "dist/react/index.js" (production)	2.73 KB (0%)
import { useSubscription } from "dist/react/index.js"	3.19 KB (0%)
import { useSubscription } from "dist/react/index.js" (production)	2.38 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js"	5.28 KB (0%)
import { useSuspenseQuery } from "dist/react/index.js" (production)	3.94 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js"	4.75 KB (0%)
import { useBackgroundQuery } from "dist/react/index.js" (production)	3.4 KB (0%)
import { useLoadableQuery } from "dist/react/index.js"	4.97 KB (0%)
import { useLoadableQuery } from "dist/react/index.js" (production)	3.63 KB (0%)
import { useReadQuery } from "dist/react/index.js"	3.04 KB (0%)
import { useReadQuery } from "dist/react/index.js" (production)	2.98 KB (0%)
import { useFragment } from "dist/react/index.js"	2.18 KB (0%)
import { useFragment } from "dist/react/index.js" (production)	2.13 KB (0%)

[jerelmiller]

/release:pr

[github-actions]

A new release has been made for this PR. You can install it with npm i @apollo/[email protected].

[phryneas]

Tbh., I'm not 100% sure if I like this fix.

For one, I don't 100% agree with the new behavior - but I'll let that aside for now, but it would be good if we discussed that in person. (Adding it to the agenda)

What's more important to me is: we call internalState.executeQuery only once - why do we end up with two network requests? That looks like a bug at some "deeper" place, and this fix seems more like it circumvents that than fixing the actual bug. Or am I missing something here?

PS: I think the core problem is more that resubscribeAfterError here triggers a full network request.
https://app.replay.io/recording/vit9448-2--f7d74d26-3bd1-456d-8fb8-6ba7f99577c2?commentId=&focusWindow=eyJiZWdpbiI6eyJwb2ludCI6IjAiLCJ0aW1lIjowfSwiZW5kIjp7InBvaW50IjoiMjM2ODk4NTQ0MTc0MDk5OTkxMDA1NzAzMzQyODczNjQwOTYiLCJ0aW1lIjoxNDY1NH19&point=14603334914963160254108903973847201&primaryPanel=debugger&secondaryPanel=console&time=10045.457486520116&viewMode=dev

[jerelmiller]

After discussing with @phryneas in person and going back through old versions (tested from 3.0 to 3.3), the behavior I've added in this PR differs from historical versions. Previously variables were only merged at the point when the execute function was called, but the values were never maintained across renders. This means that calling execute again with no variables after calling it with variables previously would instead send empty variables in the network request. I'll be updating this to restore the previous behavior to prevent an accidental breaking change in behavior.

[phryneas]

With the type changes that change the type signature of useStableCallback/useEvent, this call would look like this.

[phryneas]

What's your take on using an useEvent here, too, to keep the callback perfectly stable like it was before?
That would also make a lot of the efforts to keep things stable above obsolete.

[jerelmiller]

Sometimes the answer stares you right in the face and you're too busy complicating things to notice...

Love it. Will get this updated! This should hopefully allow me to drop the useEvent calls for the other callbacks as well 🙂

[jerelmiller]

11ac51f

[phryneas]

This is so much easier to review now ❤️
I'll take another look at the tests tomorrow, but code-wise this already looks very good!

[phryneas]

I think my preference would be that we changed only the one runtime statement that is actually related to the fix in this PR, and that we opened a second PR to introduce useEvent and fixed the other open issue from #11511, as this PR doesn't address all of them in it's current state anyways.

[phryneas]

I have one minor gripe with this PR: It feels like the whole changed code in the PR could be avoided, and we'd have to only change this one single line, without even introducing useEvent.

I'm fine with this addition if we plan on actually using useEvent, but if we will only end up using it in this hook, this might be too much noise for what could have been a very small change.

[billnbell2]

I am having a hard time knowing what version this is in ?

[jerelmiller]

@billnbell2 this was just released in 3.9.4!