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

Add cursor pagination #69

Merged
merged 11 commits into from
Oct 16, 2022
Merged

Add cursor pagination #69

merged 11 commits into from
Oct 16, 2022

Conversation

karatakis
Copy link
Collaborator

@karatakis karatakis commented Oct 4, 2022

PR Info

Adds

  • Cursor based pagination

Breaking Changes

  • Query returned object field 'data' was renamed to 'nodes'

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 5, 2022

Looks cool! Just a quick question, so instead of :

film(pagination: { limit: 10, page: 0 }, orderBy: { title: ASC }) {

we can now do:

film(cursor: {limit: 5, cursor: "Int[4]:3361"}, orderBy: { title: ASC }) {

am I right?

@karatakis
Copy link
Collaborator Author

@tyt2y3 but what about page pagination ? should we drop it ?

@karatakis karatakis force-pushed the cursor-pagination-new branch from 8b7049b to 7a9eb9c Compare October 8, 2022 09:10
@tyt2y3
Copy link
Member

tyt2y3 commented Oct 9, 2022

I imagine both cursor and pagination are optional arguments, and that they cannot be specified at the same time.

@karatakis
Copy link
Collaborator Author

@tyt2y3 I found a way to unify both APIs. But we have braking changes because instead of PaginatedResult I return Connection (async_graphql builtin type that is compatible with Relay GraphQL spec https://relay.dev/graphql/connections.htm)

@karatakis karatakis marked this pull request as ready for review October 10, 2022 19:39
@tyt2y3
Copy link
Member

tyt2y3 commented Oct 11, 2022

No problem for a breaking change as long as we update all examples and docs.

@karatakis karatakis force-pushed the cursor-pagination-new branch from e2cdff2 to 2555cbe Compare October 11, 2022 11:58
@karatakis
Copy link
Collaborator Author

Updated documentation also SeaQL/seaql.github.io#58

@tyt2y3
Copy link
Member

tyt2y3 commented Oct 16, 2022

Very cool!

@tyt2y3 tyt2y3 merged commit 126a1e3 into main Oct 16, 2022
@karatakis karatakis deleted the cursor-pagination-new branch October 16, 2022 20:31

let stmt = #path::order_by(stmt, order_by);
let cursor_string = CursorValues(values).encode_cursor();

Choose a reason for hiding this comment

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

Hey peeps, not sure if I understand the code here correctly. I think cursor should be constructed from values of sort by columns, not from PrimaryKey like here.

For example, when you sort by name, price then cursor should be combination of value of name and value of price.

Copy link
Member

Choose a reason for hiding this comment

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

Hey @phungleson, currently, in seaography, we only support cursor pagination on primary key only.

@karatakis please correct me if I'm get it wrong.

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.

Cursor based pagination Add cursor oriented pagination
4 participants