-
Notifications
You must be signed in to change notification settings - Fork 57
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
Support RecordsQuery pagination in Web5 #268
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
TBDocs Report ✅ No errors or warnings @web5/api
Updated @ 2023-11-28T23:17:03.775Z - Commit: |
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #268 +/- ##
=======================================
Coverage 91.77% 91.77%
=======================================
Files 73 73
Lines 15758 15764 +6
Branches 1448 1448
=======================================
+ Hits 14462 14468 +6
Misses 1270 1270
Partials 26 26
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LiranCohen Can you also update the README files as part of this PR once we settle on the web5.dwn.*
API syntax?
packages/api/src/dwn-api.ts
Outdated
@@ -353,7 +356,7 @@ export class DwnApi { | |||
agentResponse = await this.agent.processDwnRequest(agentRequest); | |||
} | |||
|
|||
const { reply: { entries, status } } = agentResponse; | |||
const { reply: { entries, status, paginationMessageCid } } = agentResponse; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@LiranCohen In the hopes of providing the most intuitive developer experience, I was thinking that perhaps we name paginationMessageCid
something that alludes to this value being the pagination cursor.
I was reading through the PR that originally introduced the pagination feature. I can understand the reasons for using the messageCid
as the token but it seems like providing a more descriptive name to the value returned would have been more intuitive for developers using the feature. Particularly since so many other pagination implementations often return to the value as a token
or cursor
.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree I think what we landed on wrt the cursor objects is less than optimal, especially looking at it again after some time.
I'm leaning towards a more generic naming, and we can define the explicitness of the values within docs/spec/comments.
Example:
// Query pagination object in descriptor
export type Pagination = {
cursor?: string
limit?: number
};
export type RecordsQueryResponse = {
status: UnionMessageReply['status'];
records?: Record[];
cursor?: string;
};
If we really want to be explicit about the cursor being a messageCid
, which could have some value, maybe something like cursorMessageCid
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. How do we move forward? Is there an update to dwn-sdk-js
and then web5-js
? Or another approach you prefer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@frankhinek Originally was thinking of making the change here first to get it in, but then realized that it would be a hassle since the pagination object with messageCid
is in the descriptor of the request.
I opened a PR in dwn-sdk-js
to get this in and bubble it up to here.
807622a
to
e7798ab
Compare
@frankhinek updated this PR and added the missing documentation. Although would love some suggestions on the wording, not my strongest suit. Also, not sure that I saw a pattern anywhere to document |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚀
…g pagination/sort
Signed-off-by: Frank Hinek <[email protected]>
23f993b
to
5b81224
Compare
* Add pagination messageCid to RecordsQueryResponse and tests supporting pagination/sort * cursor instead of paginationsMessagecid --------- Signed-off-by: Frank Hinek <[email protected]> Co-authored-by: Frank Hinek <[email protected]>
* Add pagination messageCid to RecordsQueryResponse and tests supporting pagination/sort * cursor instead of paginationsMessagecid --------- Signed-off-by: Frank Hinek <[email protected]> Co-authored-by: Frank Hinek <[email protected]>
Add
paginationMessageCid
to theRecordsQueryResponse
object, now when providing a limit, if there are additional results this cursor will be returned to be used in subsequent queries.Example response with additional results:
Example request for additional results:
Added a pagination test, as well as a missing sorts test.
@frankhinek I'm not sure if this is the best place to test these particular paths, input would be appreciated.