Refactor useLazyQuery to reuse useInternalState and make execution function call reobserve instead of refetch
#9564
Conversation
| function useInternalState<TData, TVariables>( | ||
| export function useInternalState<TData, TVariables>( |
There was a problem hiding this comment.
This function needs to be exported from this module so useLazyQuery can use it, but it is not exported publicly from the @apollo/client/react package.
| @@ -466,7 +480,7 @@ class InternalState<TData, TVariables> { | |||
| }; | |||
| } else if ( | |||
| this.queryHookOptions.skip || | |||
| this.queryHookOptions.fetchPolicy === 'standby' | |||
| this.watchQueryOptions.fetchPolicy === 'standby' | |||
There was a problem hiding this comment.
@benjamn I’m not 100% sure on why we read queryHookOptions over watchQueryOptions. You probably have explained this to me at some point and I just forgot.
There was a problem hiding this comment.
The this.queryHookOptions object is just the raw QueryHookOptions passed to useQuery, which we save but do not modify/normalize. My intuition tells me we should prefer this.watchQueryOptions.fetchPolicy (as normalized in createWatchQueryOptions) because it has a better chance of matching up with this.queryHookOptions.skip, possibly even making it unnecessary to check this.queryHookOptions.skip here at all. I'm not sure that intuition is grounded in anything, though, so take with 🧂.
This minimal initial commit should unlock further simplifications.
This is the most important commit of the PR, since it fixes issue #9375.
This tweak will allow us to move other behavior into the wrapped methods, like the EAGER_METHODS logic from useLazyQuery.
benjamn
force-pushed
the
useLazyQuery-useInternalState
branch
from
172d08f to
89af070
Compare
Resolves #9564 (comment) I also renamed the initialPolicy field of NextFetchPolicyContext to be named initialFetchPolicy, for consistency.
| /** | ||
| * Defaults to the initial value of options.fetchPolicy, but can be explicitly | ||
| * configured to specify the WatchQueryFetchPolicy to revert back to whenever | ||
| * variables change (unless nextFetchPolicy intervenes). | ||
| */ | ||
| initialFetchPolicy?: WatchQueryFetchPolicy; |
There was a problem hiding this comment.
I don't love adding another optional field to WatchQueryOptions, but I truly do not expect anyone to need to use this field directly, and it helps us maintain the promise that skip: true is (more or less) synonymous with fetchPolicy: "standby", because it gives us a way to pass fetchPolicy: "standby" when skip: true without ending up stuck with initialFetchPolicy === "standby".
benjamn
changed the base branch from
replace-useQuery-options-functions-with-options.defaultOptions
to
release-3.6
| // When skipping, we set watchQueryOptions.fetchPolicy initially to | ||
| // "standby", but we also need/want to preserve the initial non-standby | ||
| // fetchPolicy that would have been used if not skipping. | ||
| Object.assign(watchQueryOptions, { | ||
| initialFetchPolicy, | ||
| fetchPolicy: 'standby', | ||
| }); |
There was a problem hiding this comment.
This is where the initialFetchPolicy option comes in handy, because ObservableQuery can no longer use the fetchPolicy to infer the initialFetchPolicy if we use fetchPolicy: 'standby' to represent skip: true, so it's important to have another way to specify the initialFetchPolicy.
Building on #9563 and taking inspiration from #9459, this refactoring allows
useLazyQueryto use the sameInternalStatemachinery asuseQuery(introduced in #9459), and fixes issue #9375 by making the execution function callreobserve(which respects the currentfetchPolicy) instead ofrefetch(which always triggers a network request).