Skip to content
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

Deprecate fetchMore updateQuery function, and provide basic field policy helper functions #6464

Merged
merged 7 commits into from
Jun 22, 2020

Conversation

benjamn
Copy link
Member

@benjamn benjamn commented Jun 19, 2020

What began as an attempt to fix #5951 ultimately turned into the realization that the updateQuery function used by fetchMore is worse in almost every way than an equivalent field policy merge function.

Not only do you have to pass updateQuery every time you call fetchMore, but that repetition encourages a dangerously simplistic strategy of just concatenating the arrays together.

By contrast, a field policy needs to be defined only once, operates at the field level rather than the query level, and has a chance to examine field arguments to guide the merging process. Also, the keyFields: false default configuration allows the field policy to avoid the awful fetchMore hack of cramming additional results into the original array (keyed by the original arguments, which fetchMore does not update).

Since we are in the RC phase of AC3 testing, we are not removing or changing the behavior of updateQuery at this time, but a warning will be displayed (in development, at most once) to encourage switching to a field policy.

The tests included in this PR contain numerous side-by-side examples of the transition from updateQuery to a field policy, using both concatPagination and offsetLimitPagination.

@benjamn
Copy link
Member Author

benjamn commented Jun 19, 2020

@hwillson @jcreighton In observation of Juneteenth, please don't worry about reviewing this today!

Copy link
Member

@hwillson hwillson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome @benjamn, and I love the new pagination helpers! I think we're good to LGTM (pending a quick CHANGELOG update mentioning the updateQuery deprecation and new pagination helpers). Thanks!

benjamn and others added 4 commits June 22, 2020 11:16
I made this helper function an export of the the @apollo/client/utilities
entry point so it will not be included in your application bundle unless
you import and use it.

We hope you will find it useful, but we also encourage using it just for
guidance/inspiration when writing your own custom field policy helper
functions. For example, you might want to change args.offset to
args.start, or args.limit to args.max.
This pagination helper policy is flawed because it does not consider any
field arguments, but it seems useful for AC3 migration purposes, because
it does exactly what a typical fetchMore updateQuery function would do,
just a little more reliably.
This commit also deprecates the updateQuery function, while preserving its
existing behavior for backwards compatibility. If you're using a field
policy, you shouldn't need an updateQuery function.

Fixes #5951.
@benjamn benjamn force-pushed the 5951-fix-fetchMore-merge-variables branch from 9f89355 to 510b96a Compare June 22, 2020 15:17
Once we remove the deprecated updateQuery function, we can remove those
tests, but it seems important to test both styles for now.
@benjamn benjamn force-pushed the 5951-fix-fetchMore-merge-variables branch from 510b96a to f3676b6 Compare June 22, 2020 15:17
@benjamn benjamn changed the title Deprecate fetchMode updateQuery function, and provide basic field policy helper functions Deprecate fetchMore updateQuery function, and provide basic field policy helper functions Jun 22, 2020
@benjamn benjamn merged commit dbea4db into master Jun 22, 2020
@benjamn benjamn deleted the 5951-fix-fetchMore-merge-variables branch June 22, 2020 15:50
expect(data).toEqual(carResults);
fetchMore({
variables: {
limit: 1
Copy link
Contributor

@henryqdineen henryqdineen Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it realistic to call fetchMore() with the same args as the initial query? I assumed in this case there would be an offset argument or something. In that case would we need a custom read() function for this field? Thanks.

Copy link
Contributor

@henryqdineen henryqdineen Jun 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind. I just realized how keyArgs: false allows this to work. Sorry to bother.

@rRaDuCaN
Copy link

Hi everyone, I want to create sort of pagination by leveraging fetchMore, it kind of works in some way but is not acceptable. Here is what I'm doing:
-> I got some departments with tons of products described in a query type Subdepartment {
name: String
limit: String
page(p: $p): [String]
}
By default, useQuery fetches the p1 page, const {data} = useQuery(aQuery, {variables: {p: 'p1'}, onComplete(){...}});
That's how fetchMore looks: fetchMore({query: aQuery}, variables: {p: "p2"});
That's the field policy for the query:

typePolicies: {
Query: {
fields: {
shopHomeFurnishing: {
merge(
existing = {},
incoming,
{ mergeObjects }
) {
console.log('existing: ', existing);
console.log('incoming: ', incoming);
return mergeObjects(existing, incoming);
}
},
}
}
}
Besides that I have some client state in this typePolicy

How it should work:
image
The acual behaviour:
image
the query is requested twice,
the first time: with p = "p2" variable
and after it: with p = "p1" variable
It's like first runs fetchMore, then after again useQuery.
image
However if I disable store initialization for fields with @client directory everything works. Anyone has a hint on it?

@Felix-Indoing
Copy link

While using 'fetchMore' bundle splitting is obvious since all the logic is inside the lazy components, which will be loaded as needed. If the user do not use the components, their related logic will not download.

How can we achieve code splitting with centralized field policy?

@benjamn
Copy link
Member Author

benjamn commented Aug 12, 2020

@Felix-Indoing We are currently working on some documentation for that, though please read my review comments as well as the current draft documentation: #6766

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cache FieldPolicy.merge receives stale args when using fetchMore
5 participants