-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Provide more context to nextFetchPolicy
functions
#9222
Conversation
src/core/watchQueryOptions.ts
Outdated
export interface NextFetchPolicyContext<TData, TVariables> { | ||
reason: | ||
| "after-fetch" | ||
| "variables-changed" | ||
observableQuery: ObservableQuery; | ||
options: WatchQueryOptions<TVariables, TData>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect this interface to continue growing as we come up with new reason
s for applying nextFetchPolicy
functions, and/or additional contextual information to help nextFetchPolicy
make good decisions.
1b63aaf
to
1447728
Compare
} else { | ||
options.fetchPolicy = options.nextFetchPolicy; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this change affect this scenario described here?
Wondering the behavior described in 4
will remain consistent:
Discovered that setting fetchPolicy in the useQuery hook, would implicitly set nextFetchPolicy with the same policy, but only if there were no defaultOptions set in the ApolloClient instance.
Thanks for your time and confirmation here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dancrew32 Since you asked this question, I added some tests to verify the behavior should be the same as before, though it's possible I haven't understood/captured exactly what you mean. The tests exercise defaultOptions.watchQuery.nextFetchPolicy
in a number of ways, for example. Please take a look at the new tests in src/core/__tests__/fetchPolicies.ts
, try modifying them if you like, and let us know if anything seems worth adding to improve the test suite.
One difference that might be worth calling out: if (and only if) you were already using a nextFetchPolicy
function (uncommon, but supported since #6893 or AC v3.2.0), that function will now be called to handle the variables-changed
event as well as the after-fetch
event, whereas previously the fetchPolicy
was reset to its initial value when variables
changed with no chance to intercept that (possibly unwanted) behavior. I'm mildly concerned this could be a breaking change for existing nextFetchPolicy
functions that are not prepared to be called in the variables-changed
case, though I'm hopeful most existing nextFetchPolicy
functions map their own previous result to itself (so cache-first
stays cache-first
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My organization has never needed nextFetchPolicy
in any useQuery
call sites, so this change shouldn't affect us. I can report back after we upgrade in the future, as I am interested in removing defaultOptions
overrides in our ApolloClient
instantiation. Thanks for getting back to me, testing, and documenting all of this.
1447728
to
26a893f
Compare
@StephenBarlow It may be easier to review these docs changes after this PR is merged into |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benjamn I think this looks good! Any small edits I might make can easily happen after 3.6 lands
d9c873b
to
b47283c
Compare
b47283c
to
e557dce
Compare
We introduced
options.nextFetchPolicy
in PR #6712 to allow modifyingoptions.fetchPolicy
after first use, and later made the API more powerful by allowingnextFetchPolicy
to be a function (PR #6893) which takes the currentfetchPolicy
and dynamically returns a new policy.This more advanced functional version of
nextFetchPolicy
allows implementing a basic state machine where the various fetch policies (cache-first
,network-only
, etc) are the states, and the currentfetchPolicy
is the only input/event provided. When there's only one possible event (that is, the currentfetchPolicy
was just used to fetch the first result), you could argue it doesn't have to be explicitly provided to the state transition function (nextFetchPolicy
), but then your state machine will probably be pretty limited/boring. Most state machines have several possible inputs/events triggering a variety of transitions between states, and Apollo Client is already starting to need more than onenextFetchPolicy
event (see #8465).This PR provides additional context to the
nextFetchPolicy
function as its second parameter (leaving the first parameter untouched for backwards compatibility):The
context.reason
can currently be one of the following strings:after-fetch
options.fetchPolicy
, allowing the usual transition from a network fetch policy likecache-and-network
ornetwork-only
to a more cache-friendly policy likecache-first
orcache-only
.variables-changed
context.options.fetchPolicy
gets reset tocontext.initialPolicy
, as implemented in Improve processing of options.nextFetchPolicy #8465 to fix [v3.4 Regression] Changing variables uses nextFetchPolicy instead of fetchPolicy #8426. If you provide anextFetchPolicy
transition function, you can override this behavior by ignoring thevariables-changed
event.As a consequence of these changes, one way to make sure
nextFetchPolicy
sticks permanently is to pass a function that ignores the providedfetchPolicy
andcontext
and always returns the same constant value:This is different from
because the second version allows
options.fetchPolicy
to be reset back tocache-and-network
whenoptions.variables
change, whereas the first will continually enforcecache-first
.This PR is still a draft because it obviously needs more tests and documentation.