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

ApolloPagination usage docs #262

Merged
merged 31 commits into from
Feb 23, 2024
Merged

Conversation

Iron-Ham
Copy link
Contributor

@Iron-Ham Iron-Ham commented Feb 9, 2024

This PR added ApolloPagination to the list of valid targets for the DocumentationGenerator command. I added the ApolloPagination package as a dependency of the project.

NOTE: I am receiving the following warning –

warning: 'apollo-ios-pagination': 'apollo-ios-pagination' dependency on 'https://github.com/apollographql/apollo-ios.git' conflicts with dependency on '/Users/zer0/Developer/OSS/apollo-ios-dev/apollo-ios' which has the same identity 'apollo-ios'. this will be escalated to an error in future versions of SwiftPM.

This PR also adds a new pagination entry in the sidebar & a short intro md.
For now, I've left the visibility of the pagination entry as false.

I wasn't able to get this running locally, so I have no clue what this would look like in the doc portal.

@Iron-Ham Iron-Ham requested a review from a team as a code owner February 9, 2024 22:21
Copy link

netlify bot commented Feb 9, 2024

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

Visit the deploys page to approve it

Name Link
🔨 Latest commit 2581199

@Iron-Ham Iron-Ham force-pushed the hs/generate-docc branch 2 times, most recently from 652cfb8 to 9162553 Compare February 10, 2024 03:04
@Iron-Ham Iron-Ham changed the title Add ApolloPagination to the DocC generation project Add ApolloPagination to DocC generation, generate short Introduction Feb 10, 2024
Copy link
Contributor

@AnthonyMDev AnthonyMDev left a comment

Choose a reason for hiding this comment

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

This is an awesome start! I've got a few minor suggestions and questions.

I also want to start a running list of things to be added to documentation (can be done in the future):

  • Example of Reverse/Bidirectional Pagination
  • Example of Offset based pagination
  • Example of using different queries for initial query vs additional pages

docs/source/pagination/introduction.mdx Outdated Show resolved Hide resolved
docs/source/pagination/introduction.mdx Outdated Show resolved Hide resolved
docs/source/pagination/introduction.mdx Outdated Show resolved Hide resolved
docs/source/pagination/introduction.mdx Outdated Show resolved Hide resolved
docs/source/pagination/introduction.mdx Outdated Show resolved Hide resolved
docs/source/pagination/introduction.mdx Outdated Show resolved Hide resolved
@Iron-Ham
Copy link
Contributor Author

Happy to address these – @AnthonyMDev do you have advice for running this locally, so I can see how the changes would present on the web? I wasn't able to get it running locally, which certainly made things a little tougher.

@AnthonyMDev
Copy link
Contributor

Our docs deployment repo is actually public, so you can clone it, and then use the instructions here to run the docs locally.
https://github.com/apollographql/docs?tab=readme-ov-file#developing-a-single-docset

@Iron-Ham
Copy link
Contributor Author

Our docs deployment repo is actually public, so you can clone it, and then use the instructions here to run the docs locally. https://github.com/apollographql/docs?tab=readme-ov-file#developing-a-single-docset

mvp

@Iron-Ham Iron-Ham changed the title Add ApolloPagination to DocC generation, generate short Introduction ApolloPagination usage docs Feb 13, 2024
@Iron-Ham Iron-Ham marked this pull request as draft February 13, 2024 21:19
@Iron-Ham Iron-Ham marked this pull request as ready for review February 13, 2024 22:21
@Iron-Ham
Copy link
Contributor Author

Iron-Ham commented Feb 13, 2024

cc: @AnthonyMDev I've gone ahead and added 3 more pages & added a single example of an offset pager within the Introduction. However, in writing the offset docs, i recognized there are some improvements and conveniences afforded to cursor pagination that are not extended out to offset. I'll correct that shortly.

docs/source/pagination/introduction.mdx Outdated Show resolved Hide resolved
docs/source/pagination/introduction.mdx Outdated Show resolved Hide resolved
docs/source/pagination/directional-pagers.mdx Outdated Show resolved Hide resolved
docs/source/config.json Outdated Show resolved Hide resolved
@@ -27,7 +27,7 @@ public extension GraphQLQueryPager {
client: ApolloClientProtocol,
watcherDispatchQueue: DispatchQueue = .main,
initialQuery: InitialQuery,
pageResolver: @escaping (P, PaginationDirection) -> InitialQuery,
pageResolver: @escaping (P, PaginationDirection) -> InitialQuery?,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AnthonyMDev I can pull this out of this PR if you prefer and into a second – but I noticed that the pageResolver wasn't expecting an Optional<InitialQuery> in the GraphQLQueryPager definitions, but was in the AsyncGraphQLQueryPager definitions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nah, that's fine!

Copy link
Contributor

@AnthonyMDev AnthonyMDev left a comment

Choose a reason for hiding this comment

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

Wow, reading through these docs, this library is just so incredibly feature rich already! What an awesome job @Iron-Ham!

Couple of edits to add some more information and minor fixes!

docs/source/pagination/introduction.mdx Outdated Show resolved Hide resolved
client: client,
initialQuery: initialQuery,
pageResolver: { page, paginationDirection in
// As we only want to support forward pagination, we can return `nil` for reverse pagination
Copy link
Contributor

Choose a reason for hiding this comment

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

For the convenience functions that only support single direction paging, can we just omit the pagination direction from the pageResolver closure signature? It should only be needed for bidirectional paging, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed this on our call just now – but this can be an improvement for 0.2.x

We could do it by declaring ForwardPageInfo and ReversePageInfo protocols, and then declaring the convenience functions around those protocols.

docs/source/pagination/introduction.mdx Outdated Show resolved Hide resolved
docs/source/pagination/custom-types.mdx Outdated Show resolved Hide resolved
docs/source/pagination/custom-types.mdx Outdated Show resolved Hide resolved
docs/source/pagination/directional-pagers.mdx Outdated Show resolved Hide resolved

## Forward Pagination

Forward pagination is the most common form of pagination. It is used to fetch the next `n` items in a list. We can use the convenience `make` functions to create a configured `GraphQLQueryPager`. While we have many options depending on our requirements -- whether we use one query or two, whether we want to use a cursor or an offset, whether we want to transform the results, etc. -- we will examine using a cursor with a single query.
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's talk about my question related to omitting paginationDirection in single directional convenience functions. If we can go that route, then I have some suggestions to tweak this.

docs/source/pagination/multi-query.mdx Outdated Show resolved Hide resolved
docs/source/pagination/multi-query.mdx Outdated Show resolved Hide resolved
docs/source/pagination/multi-query.mdx Outdated Show resolved Hide resolved
@@ -237,6 +237,8 @@ public class AsyncGraphQLQueryPager<Model>: Publisher {
/// Resets pagination state and cancels further updates from the pager.
public func cancel() async {
await pager.cancel()
_subject.send(completion: .finished)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to update unit tests for this? Can also be done in a separate PR later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that'd be wise, yes.
As written, I think this would require us to re-init a new pager – in the same sense that cancelling a watcher stops it from being used again.


## Cancelling ongoing requests

The `GraphQLQueryPager` class provides a `cancel` method, which can be used to cancel any ongoing fetch operations as well as cease all subscriptions. Once cancelled, the pager's state is reset. Any subscriber that was subscribed to the pager will receive a termination signal, and will no longer receive updates. After calling the `cancel` method, you msut resubscribe to the pager in order to receive updates.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The `GraphQLQueryPager` class provides a `cancel` method, which can be used to cancel any ongoing fetch operations as well as cease all subscriptions. Once cancelled, the pager's state is reset. Any subscriber that was subscribed to the pager will receive a termination signal, and will no longer receive updates. After calling the `cancel` method, you msut resubscribe to the pager in order to receive updates.
The `GraphQLQueryPager` class provides a `cancel` method, which can be used to cancel any ongoing fetch operations as well as cease all subscriptions. Once cancelled, the pager's state is reset. Any subscriber that was subscribed to the pager will receive a completion signal, and will no longer receive updates. After calling the `cancel` method the pager is invalid. You will need to create a new pager in order to receive updates again.

@AnthonyMDev AnthonyMDev merged commit 6e32e88 into apollographql:main Feb 23, 2024
14 checks passed
BobaFetters pushed a commit that referenced this pull request Feb 23, 2024
BobaFetters pushed a commit to apollographql/apollo-ios-pagination that referenced this pull request Feb 23, 2024
BobaFetters pushed a commit that referenced this pull request Feb 23, 2024
14038690  usage docs (#262)

git-subtree-dir: apollo-ios-pagination
git-subtree-split: 1403869013cfbe8cdf565cec39744d24b447fba7
BobaFetters pushed a commit that referenced this pull request Feb 23, 2024
git-subtree-dir: apollo-ios-pagination
git-subtree-mainline: bda25cd
git-subtree-split: 1403869013cfbe8cdf565cec39744d24b447fba7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants