feat: async query pager fetch with cache policy

[pondorasti]

resolves apollographql/apollo-ios#3418

[netlify]

👷 Deploy request for apollo-ios-docc pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	17f6ece

[netlify]

👷 Deploy request for eclectic-pie-88a2ba pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	17f6ece

[BobaFetters]

@AnthonyMDev @Iron-Ham Any reason either of you have that we wouldn't want to do something like this?

[Iron-Ham]

@AnthonyMDev @Iron-Ham Any reason either of you have that we wouldn't want to do something like this?

I'd love to learn more about this use-case.

I have some concerns that this would result in unexpected outcomes (when fetched using returnCacheDataDontFetch or returnCacheDataElseFetch) since the cached pagination information may rely on unstable information (offsets, some cursor implementations). We previously defaulted these methods to their current values but allowed users to supply their own cache policy. That was removed when the inconsistencies became known.

Is the idea that this data has already been fetched elsewhere in the app (in an existing session) and should be reused? If that's the case, having the second usage sink/subscribe onto the same instance of the pager would yield that data to the subscriber.

Otherwise... I think we could accomplish the same behavior by removing the assert from refetch, which allows users to specify their policy?

[pondorasti]

Is the idea that this data has already been fetched elsewhere in the app (in an existing session) and should be reused?

The app is structured around many View/ViewModel components that are more or less tied to a feature. There's very few environment objects passed down (i.e. user model). So each part of the app defines it's own GraphQL queries and heavily relies on the normalized cache to avoid a crazy number of fetch requests. Also since all the queries are implemented with watchers it's very easy to keep state in sync when the normalized cache is updated directly or indirectly.

In the particular case of pagination, we have a view that can be presented through multiple entry points like you mentioned. One thing to keep in mind though with the View/ViewModel pattern described above is that when views are popped from the screen, swift also garbage collects any state and view models that came with them. So even when you go back and forth on the same View, we end up triggering a bunch of fetch requests with the current apollo pagination package.

Otherwise... I think we could accomplish the same behavior by removing the assert from refetch, which allows users to specify their policy?

This would be just as good as my modification to the fetch API. Before I created this PR I did try making everything based on refetch(cachePolicy: returnCacheDataDontFetch) but the asserts where getting in the way so I opened this PR.

[Iron-Ham]

Gotcha. Thank you for the additional context @pondorasti.

I'm happy with this feature with a few changes:

• Can we instead just remove the asserts from refetch?
• Can we make sure this applies to both the GraphQLQueryPager and AsyncGraphQLQueryPager?

[pondorasti]

Can we instead just remove the asserts from refetch?

Done!

Can we make sure this applies to both the GraphQLQueryPager and AsyncGraphQLQueryPager?

Looks like GraphQLQueryPagerCoordinator is built on top of the AsyncGraphQLQueryPagerCoordinator, so removing the assert from the async pager will propagate to the sync pager too.

apollo-ios-dev/apollo-ios-pagination/Sources/ApolloPagination/GraphQLQueryPagerCoordinator.swift

Lines 39 to 41 in 94be720

	/// Handles pagination in the queue by managing multiple query watchers.
	class GraphQLQueryPagerCoordinator<InitialQuery: GraphQLQuery, PaginatedQuery: GraphQLQuery>: PagerType {
	let pager: AsyncGraphQLQueryPagerCoordinator<InitialQuery, PaginatedQuery>

[Iron-Ham]

Looks good to me! I'm happy with the assert removal as a path forward. It's a bit less opinionated, and makes the minimal change to get this feature in place.

What do y'all think @BobaFetters @AnthonyMDev?

[AnthonyMDev]

Nice and simple! Thanks for working through this @pondorasti and @Iron-Ham!