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

Create a third options argument and allow users to specify a maxPages setting #163

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jetzhou
Copy link

@jetzhou jetzhou commented Feb 13, 2024

Resolves #165


Before the change?

  • There is no way to stop the pagination early even if the user doesn't need the full list of items.

After the change?

  • An optional third parameter is added that allows users to specify a maxPages setting. This will allow users to take advantage of the auto-merging, but stop the iteration early if needed.

Pull request checklist

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

  • Yes
  • No

As a side note, I've also considered adding the maxPage into the existing GraphQL parameter object. That feels a bit dirty because maxPages doesn't really have anything to do with the query itself. Adding a third parameter seems like a cleaner API.

Copy link

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

@gr2m
Copy link
Contributor

gr2m commented Feb 13, 2024

You can implement max pages using the iterator https://github.com/octokit/plugin-paginate-graphql.js?tab=readme-ov-file#octokitgraphqlpaginateiterator, would that be an option? Using iterators is recommended anyway, as it's much more memory efficient.

@jetzhou
Copy link
Author

jetzhou commented Feb 13, 2024

Yes, using the iterator can certainly achieve the same goal but then the user will most likely have to do manual merging. I figured this is way more convenient and users won’t have to change the flow/structure of their code if they simply wanted to add a limit later.

@gr2m
Copy link
Contributor

gr2m commented Feb 13, 2024

I'm reluctant to add this new API because it diverges from what we have in the REST API pagination method:
https://github.com/octokit/plugin-paginate-rest.js?tab=readme-ov-file#octokitpaginate

It's fairly simple to add this API. But once we do, it will be much hard to change. We can see what others are thinking but that's my point of view.

Maybe create an issue describing what you would like to achive, leave the pull request open for context, but let's have a higher-level discussion about it first

@jetzhou
Copy link
Author

jetzhou commented Feb 15, 2024

That's totally fair!

I was not aware of the related API on the REST side. Looking at it, I do actually like that it's more flexible in terms of when done can be called. Their example of looking for a particular item and stopping early seems useful too.

However, in the GraphQL world, calling this function a mapping function isn't as applicable, since the query itself should already be specifying only the relevant fields.

An analogous API would work out to be something like this:

To stop after a certain number of pages:

const maxPages = 2;
let pages = 0;

await octokit.graphql.paginate(
  `query paginate ($cursor: String) {
    repository(owner: "octokit", name: "rest.js") {
      issues(first: 10, after: $cursor) {
        nodes {
          title
        }
        pageInfo {
          hasNextPage
          endCursor
        }
      }
    }
  }`,
  {},
  (_, done) => {
    pages += 1;
    if (pages >= maxPages) {
      done();
    }
  },
);

Or, to stop after you find a certain item:

await octokit.graphql.paginate(
  `query paginate ($cursor: String) {
    repository(owner: "octokit", name: "rest.js") {
      issues(first: 10, after: $cursor) {
        nodes {
          title
        }
        pageInfo {
          hasNextPage
          endCursor
        }
      }
    }
  }`,
  {},
  (response, done) => {
    if (response?.repository?.issues?.nodes?.[0].title === "Issue 2") {
      done();
    }
  },
);

I will create an issue with these options listed and see if there are strong opinions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request
Projects
Status: 🛑 Blocked/Awaiting Response
Development

Successfully merging this pull request may close these issues.

[FEAT]: Give user control to stop pagination earlier
3 participants