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

refactor(graphql): improve queries by returning a simple cursor #26

Merged
merged 3 commits into from
Dec 5, 2023

Conversation

nobe4
Copy link
Contributor

@nobe4 nobe4 commented Nov 30, 2023

The membersWithRole connection has a pageInfo containing the endCursor. It is equivalent to look at the last edge's cursor but creates a smaller return object.

This update the queries, code and fixtures to reflect the new way to paginate.

Before:

image

After:

image

Proof the cursor is the same using https://docs.github.com/en/graphql/overview/explorer:
image

The `membersWithRole` connection has a `pageInfo` containing the
`endCursor`. It is equivalent to look at the last edge's cursor but
creates a smaller return object.

This update the queries, code and fixtures to reflect the new way to
paginate.
@jasonmacgowan
Copy link
Contributor

Should we expose hasNextPage too?

@nobe4
Copy link
Contributor Author

nobe4 commented Nov 30, 2023

That's a good question, given that I'm going to fix the acceptance tests as well, I'd say let's keep this PR as small as possible and see if it's needed.

The current pagination logic works without hasNextPage, so we can leave it out for now.

nobe4 added 2 commits December 1, 2023 11:16
This corresponds to the change in the previous commit and will bridge
the gap between the test and new data format.
Copy link
Member

@northrup northrup left a comment

Choose a reason for hiding this comment

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

Nice!

@nobe4 nobe4 merged commit d39f418 into main Dec 5, 2023
7 checks passed
@nobe4 nobe4 deleted the simplify_graphql_paging branch December 5, 2023 08:37
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.

3 participants