feat: initial version

[davelosert]

This is the first implementation for my simple solution to a pagination as discussed in octokit/graphql #61.

I am still actively working on this, so it's only a draft for now - but I thought I'd create the PR for visibility purposes.

Open Todos

• Make passing initial arguments possible
• Make passing own query-statement (query myQueryName(variable1: String ...)) possible
• Adds possibility to paginate on edges (currently only nodes)
• Implement iterator function
• Allow startCursor and hasPreviousPage for backwards pagination
• Warn when Number of cursors is greater than pageInfo length (forgot page infos in query, tried nested cursors)
• Error when Cursor-Values do not change between page requests but hasNextPage === true to avoid infinite querying (when both, mixing up cursor order as well as trying nested pagination)
• Write tests for 404 or 500 cases so that they get passed through correctly
• Write documentation
• Write E2E Test
• Allow non-paginated queries (currently errors as an empty query statement is created)
• Improve Error Message if PageInfo can not be found where expected

Overall status

I'd say about 80% of this are done 🚀 - though it seems like the above list is huge, the heavy lifting is done 🏋🏻 and it currently is in a working state if used as intended and with the limitations (e.g. only endCursor and nodes allowed etc.).

Now it is mostly about DX and Documentation, so I'll try to finish this up within the week so maybe this can be shipped before friday.

closes #1
closes #3

[davelosert]

Hi @gr2m and thank you for the review! 🙂

There is an tradeoff of bundle size here, more than with other plugins, as we would make this plugin part of octokit by default. I cannot recall that I need parallel pagination. If we can remove a significant amount of code / complexity, I'd prefer to remove the support for parallel pagination.
We can work on dedicated plugins for parallel pagination and for nested pagination and reference them in the readme.

Agreed. I'll remove the feature and check how much complexity / code can be thrown out.

I'd like to test the plugin and do a thorough code review first. If the release of the plugin is a blocker, we can release beta versions until we figure out the remaining points?

Sure thing, for me there is no blocker. But having a beta release would be great to let other's test this as well (I already know there are some people who'd like to do so).

I have one general question: can you elaborate on why you decided to go with the callback method instead of relying on cursor_* variable name conventions as discussed at octokit/graphql.js#61 (comment)?

To me, it seemed like the 'less magical' and less error prone approach, especially with parallel pagination, as one would rely less on convention and more on putting the cursors yourself in the right positions. But I think it also still came a bit from my misconception that I could be delivering nested pagination with this.

From the learnings now, and given that we might not want to support parallel pagination here, I agree that it is probably easier to just set the convention that the cursors name must be '$cursor'.

The only question that remains for me here then is if we make the user tell us where to find the paginated resources, so if

1. the developers have to give us a cursor path additionally or
2. if we keep my method of simply walking over the object until a pageInfo object is found.

From DX, I'd prefer 1, but if we talk bundle size here again, I could delete the entire visit method and thus simplify both the mergeResponse as well as the extractPageInfo functions if we did 2.

I'll try it out and measure the difference in bundle size here to have more facts for what this means.

[davelosert]

Bundle Size Comparison

I'll use this issue and update this with the measured bundle-size of the original, cursor-name-based version, the one without parallel pagination support and the one with having the cursorPath be assigned by the user.

Method	Bundle Size	Minified Bundle Size
Original	8150 B	3324 B
Cursor-Convention	5930 B	2656 B
cursorPath	5990 B	2817 B

[gr2m]

the developers have to give us a cursor path additionally or

the idea is that the path is provided by the cursor_ variable name. So instead of requiring an additional { cursorPath: 'repository.stargazers' } option, the name of the cursor variable is significant, in this case cursor_repository_stargazers.

[davelosert]

The variable name still would have to be provided somehow though, no? The way this could work would be either by parsing it out of the query or by expecting the user to pass it as a variable with an initial 'null' value - or how would you do it?

[gr2m]

The variable name still would have to be provided somehow though, no?

yes, it would be passed as a query variable. The plugin could iterate through all variables, look for the ones that start with cursor_, then remove the prefix and split up the rest by _ in order to get the path.

For example in

octokit.graphql.paginate(`query($owner: String!, $repo: String!, $cursor_repository_stargazers:String) { 
  repository(name:$owner owner:$repo) {
    stargazers(first: 100, after:$cursor_repository_stargazers) {
      pageInfo {
        endCursor
        hasNextPage
      }
      nodes {
        login
        company
      }
    }
  }
}`, {
  owner: "octokit",
  repo: "plugin-paginate-graphql.js"
})

The cursor would be read out at repository.stargazers.pageInfo.endCursor and the value would be passed to the same query as cursor_repository_stargazers variable.

Do you think that'd work?

[davelosert]

Okay got it. We would have to parse it out of the provided query-string then by basically searching in the query() for something that starts with $cursor_. I guess that would work.

I'd only object hat the developer experience is a bit worse than with the more 'magical' $cursor as one can easily mistype the variable - and it becomes quite long.

But I'll try it, and if it has the same impact on the bundle size as going from the function to the pure cursor, that could be a trade off that's worth it.

[gr2m]

I'd only object hat the developer experience is a bit worse than with the more 'magical' $cursor as one can easily mistype the variable - and it becomes quite long.

yeah I'm with you, I like your approach as well, but it would be great to see the comparison.

Regarding typos, we should be able to address that concerns with well crafted error messages

[davelosert]

@gr2m : So I tried the way with having the path in the cursor in this branch here: https://github.com/davelosert/plugin-paginate-graphql.js/tree/pagination-curosr-convention

The bundle size is almost similar:

Method	Bundle Size	Minified Bundle Size
CursorPath parsed from response	5930 B	2656 B
cursorPath in Variable	5990 B	2817 B

This is probably due to needing another error from the parsing which then makes up for the little less required functionality when parsing the path from the response.

So I'd say we leave it as it is here - with the $cursor convention and parsing the path from the response.

However, I leave the last decision up to you, happy to do it both ways 🙂

Workflows

I also added the workflow-files now. Two things you would have to do:

• Fix the expected required jobs for pull requests (currently seems to be test (10), (test (12), test(14))
• Provide the OCTOKITBOT_NPM_TOKEN token in the secrets

Should I change the target to the beta branch ?

[gr2m]

we have to use master until we rename the default branch. Before we can do that, we also need to configure semantic-release in package.json, let's do that later

[gr2m]

So I'd say we leave it as it is here - with the $cursor convention and parsing the path from the response.

okay

Fix the expected required jobs for pull requests (currently seems to be test (10), (test (12), test(14))

we have a new setup where we use a matrix and a separate test job that requires the matrix to pass. Let's take care of that after the initial release.

Provide the OCTOKITBOT_NPM_TOKEN token in the secrets

I'm on it, I need GitHub security involvement, I'll have to wait for next week

[davelosert]

we have a new setup where we use a matrix and a separate test job that requires the matrix to pass. Let's take care of that after the initial release.

Yep that's what I copied over from octokit/octokit.js - the problem here was that I used main as target, that's why it didn't get picked up by this PR. Now it works.

I'm on it, I need GitHub security involvement, I'll have to wait for next week

Sounds good! 👍🏻

[gr2m]

Actually nevermind, the OCTOKITBOT_NPM_TOKEN secret is already present, it's an org secret. We should be good to go

[github-actions]

🎉 This PR is included in version 1.0.0 🎉

The release is available on:

• GitHub release
• npm package (@latest dist-tag)

Your semantic-release bot 📦🚀

[davelosert]

Great this is released, thank you for merging! 🥳

Sorry you had some troubles releasing the first version as I seem to have forgotten the @octokit scope in the package.json. But why not start with Version 2.0.0 ;)

[oscard0m]

I did not have time to give this a try but overall congrats for the great work and effort here @davelosert 👏🏽