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

More flexible cursor pagination #926

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

Conversation

henrikskog
Copy link
Contributor

@henrikskog henrikskog commented May 19, 2024

This is a suggestion for a more flexible approach to cursorbased pagination. Feedback is very welcome!

The helper function singleColPaginatedQuery is only for when the cursor consists of one column name.

See packages/core/src/modules/committee/committee-repository.ts and packages/core/src/modules/interest-group/interest-group-repository.ts for example usage.

If you want custom sorting you have to implement the pagination logic manually for that use case

Copy link

linear bot commented May 19, 2024

@henrikskog henrikskog marked this pull request as draft May 19, 2024 15:31
@henrikskog
Copy link
Contributor Author

Deprecation of old method here #927

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made this mostly for testing the pagination, I could move the pagination testing to a seperate file

Comment on lines 40 to 41
column: "id",
order: "desc",
Copy link
Member

Choose a reason for hiding this comment

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

I think the pagination should be generic and support a list of columns, it seems rather easy to do. Whether you make the API be ["X", { columnName: "Y", ordering: ASCENDING/DESCENDING }], or ["X", "-Y"] I'm somewhat indifferent about

The cursor can just be base64 of e.g. a JSON-array like ["columnAValue", "columnBValue"]

Comment on lines 43 to 44
decodeCursor?: (cursor: Cursor) => OperandValueExpression<DB, TB, C>
buildCursor?: (record: O) => Cursor
Copy link
Member

@henrikhorluck henrikhorluck May 20, 2024

Choose a reason for hiding this comment

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

These (cursor-build/decode) Should not be necessary, as far as I can tell cursor-encode/decode is only dependent on which columns you have

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you explain the use case of having individual sorting on each column? Are they not the same sorting order in 99% of the time?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm I guess if you have a table where you can sort on each column individually then it's useful.

Copy link
Member

@henrikhorluck henrikhorluck May 20, 2024

Choose a reason for hiding this comment

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

That functionality is not what I'm describing. Though it is used quite a few places in e.g. OW4's admin-panel, mostly useful when working directly towards a table.

What I'm talking about here is rather that instead of the users of this API having to specify how to encode/decode the cursor, that should be a part the implementation detail of our CursorPagination-package/module/whatever. That would make the call-locations more focused on intent, instead of technical implementation-details (I think you would call that a leaky abstraction?)

This does reduce what you can implement on top of this, but I think it is better to extend the functionality of CursorPagination rather than making call-locations more complex, and solves the general problem rather than hacking stuff together with tight coupling to e.g. our primary key-usage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just fixed default cursor-encode/decode for multiple columns.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With the current api you cannot order on individual columns when there are multiple columns spesified, as this makes things quite a bit more complex. What do you think about this @junlarsen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Take a look at the new test packages/core/src/utils/cursor.e2e-spec.ts to see the current api

packages/core/src/utils/cursor.ts Outdated Show resolved Hide resolved
packages/core/src/utils/cursor.ts Outdated Show resolved Hide resolved
packages/core/src/utils/cursor.ts Show resolved Hide resolved
packages/core/src/utils/cursor.ts Show resolved Hide resolved
packages/core/src/utils/cursor.ts Outdated Show resolved Hide resolved
packages/core/src/utils/cursor.e2e-spec.ts Show resolved Hide resolved
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