Improve processing of options.nextFetchPolicy #8465
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benjamn
force-pushed
the
ObservableQuery-nextFetchPolicy-improvements
branch
from
c2ba33f
to
e254dd4
Compare
brainkim
reviewed
Jul 8, 2021
benjamn
added a commit
that referenced
this pull request
Jul 9, 2021
benjamn
added a commit
that referenced
this pull request
Jul 9, 2021
brainkim
approved these changes
Jul 9, 2021
Despite my vague suggestion in the removed comments that deleting options.nextFetchPolicy somehow helped make the fetchPolicy transition idempotent (a concern that only matters when nextFetchPolicy is a function), leaving options.nextFetchPolicy intact should also be safe/idempotent in virtually all cases, including every case where nextFetchPolicy is just a string, rather than a function. More importantly, leaving options.nextFetchPolicy intact allows it to be applied more than once, which seems to fix issue #6839.
ObservableQuery reobserve operations can either modify this.options and
replace this.concast with a Concast derived from those modified options,
or they can use a (shallow) copy of this.options to create a disposable
Concast just for the current reobserve operation, without permanently
altering the configuration of the ObservableQuery.
This commit clarifies which operations receive fresh options (and do not
modify this.concast): NetworkStatus.{refetch,poll,fetchMore}, as decided
in one place, by the ObservableQuery#reobserve method.
This list is subject to change, and I suspect we may end up wanting to
use criteria besides NetworkStatus in some cases. For now, NetworkStatus
seems to capture the cases we care about.
Since NetworkStatus.poll operations "run independently" now, with their
own shallow copy of this.options, the forced fetchPolicy: "network-only"
no longer needs to be reset by nextFetchPolicy. More generally, since
polling no longer modifies the ObservableQuery options at all, the
interaction betwee polling and other kinds of network fetches should be
less complicated after this commit.
This change seems consistent with the goals of #7437, though variables can be changed without calling ObservableQuery#setVariables.
Specifying options.nextFetchPolicy is unnecessary because refetch operations receive a (disposable) copy of the ObservableQuery options, so options.fetchPolicy does not need to be reset.
benjamn
force-pushed
the
ObservableQuery-nextFetchPolicy-improvements
branch
from
a198072
to
8a98eb9
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These changes make the processing of
options.nextFetchPolicy(introduced in #6712) more predictable, in the following ways:options.nextFetchPolicyproperty will no longer be removed after it's applied for the first time, allowing it to continue to apply any time a differentfetchPolicyis (temporarily) used.ObservableQuery#reobservemethod is now more explicit about which kinds of reobservations get their ownoptionsandConcastobservable, so those operations (for example,refetchandpoll) no longer need to specifynextFetchPolicy(since theiroptionsare separate and will be discarded).applyNextFetchPolicyfunction is now called in only one place, fixing a regression that I identified in [v3.4 Regression] Changing variables uses nextFetchPolicy instead of fetchPolicy #8426 (comment).Fixes the following issues (full list TBD):