Avoid full reapplication of cache-and-network and network-only fetch policies after successful fetchMore
#9504
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
commented
Mar 9, 2022
benjamn
changed the title
fetchMorecache-and-network and network-only fetch policies after successful fetchMore
benjamn
commented
Mar 9, 2022
benjamn
commented
Mar 9, 2022
benjamn
force-pushed
the
avoid-full-reobservation-after-fetchMore
branch
from
5f3ea17 to
0850e3a
Compare
benjamn
force-pushed
the
avoid-full-reobservation-after-fetchMore
branch
from
c47e347 to
387fb97
Compare
benjamn
commented
Mar 10, 2022
Comment on lines
-404
to
-419
| if (__DEV__ && | ||
| !warnedAboutUpdateQuery) { | ||
| invariant.warn( | ||
| `The updateQuery callback for fetchMore is deprecated, and will be removed | ||
| in the next major version of Apollo Client. | ||
|
|
||
| Please convert updateQuery functions to field policies with appropriate | ||
| read and merge functions, or use/adapt a helper function (such as | ||
| concatPagination, offsetLimitPagination, or relayStylePagination) from | ||
| @apollo/client/utilities. | ||
|
|
||
| The field policy system handles pagination more effectively than a | ||
| hand-written updateQuery function, and you only need to define the policy | ||
| once, rather than every time you call fetchMore.`); | ||
| warnedAboutUpdateQuery = true; | ||
| } |
There was a problem hiding this comment.
Removing this warning is something a number of developers have requested (for example, this twitter discussion). At this point, I believe the warning has done all the good it can do (nudging people to stop using updateQuery and use field policies instead), so removing the warning will improve the lives of developers who still have a good reason to use updateQuery. Also, updateQuery is now implemented in terms of cache.updateQuery (#8382), which is an API we intend to continue supporting.
benjamn
force-pushed
the
avoid-full-reobservation-after-fetchMore
branch
2 times, most recently
from
e8d2536 to
3c3eb1f
Compare
benjamn
added a commit
that referenced
this pull request
Mar 11, 2022
Although these test updates may seem substantial, I believe this refactoring makes the tests more robust without changing what they test. To that end, it's important to note these tests are all passing at this point in the commit history, before any of the more substantive changes from PR #9504, and continue passing even after those changes are introduced, with relatively few additional test changes.
benjamn
force-pushed
the
avoid-full-reobservation-after-fetchMore
branch
from
3c3eb1f to
bb83bca
Compare
|
As a demonstration of the safety of the |
benjamn
added a commit
that referenced
this pull request
Mar 11, 2022
benjamn
commented
Mar 11, 2022
Although these test updates may seem substantial, I believe this refactoring makes the tests more robust without changing what they test. To that end, it's important to note these tests are all passing at this point in the commit history, before any of the more substantive changes from PR #9504, and continue passing even after those changes are introduced, with relatively few additional test changes.
Now that fetchMore's updateQuery callback is implemented in terms of the supported/documented cache.updateQuery method, I feel better about allowing fetchMore to continue to take an updateQuery callback. Also, everyone with any ability to migrate from updateQuery to InMemoryCache field policies has presumably already done so, so this warning is less useful now than it was following the release of AC3.
When `!diff.complete`, `oq.reobserveCacheFirst()` should behave exactly like `oq.reobserve()`, since the fetch policies `reobserveCacheFirst` modifies (`network-only` and `cache-and-network`) behave the same as the `cache-first` policy when cache results are incomplete.
This reverts commits d5463be and 0170f32, with a new test showing why the backup reobserveCacheFirst call in the finally block is important: sometimes the cache write doesn't change any data in the cache, so no broadcast happens, but we still need to deliver the final loading:false result for the fetchMore request.
benjamn
force-pushed
the
avoid-full-reobservation-after-fetchMore
branch
from
1bf0867 to
5036dc5
Compare
|
These changes have been published to npm as |
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.
This PR implements a variation on the idea I sketched in #6916 (comment),
without doing too much violence to our existing test suite. I did end up making a number of adjustments to tests that this PR initially caused to fail, but I'm confident they are nonviolent (that is, they test the same things in a more robust/realistic way).Previous attempts to solve #6916 stalled in part because the number of test failures felt indicative of potential backwards incompatibility. Now that I've worked through all the failing tests in this PR, I believe they are mostly just fragile/flaky/timing-sensitive tests, though of course it would be great to validate that hope with beta testing.
In short,
fetchMoreshould (now) be able to deliver complete cache results toObservableQuerysubscribers without triggering additional network requests when using thecache-and-networkornetwork-onlyfetch policies, resolving the following related (some long-standing) issues: