-
Notifications
You must be signed in to change notification settings - Fork 1
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 async iterator for query results #1198
Conversation
3309850
to
46cc2e5
Compare
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.
Really nice approach. Ideally we'd have tests that covered 0, 1, and 2 pages to account for different conditions in the generator
Yes, I'll add more unit tests for sure. And I'm not 100% sure the name of the function is descriptive of what it does, paginating results is what the server does technically, this goes through the pages, but I haven't found a more suitable name yet. |
0478b52
to
3f26cee
Compare
page = await query(page.next, options); | ||
} | ||
// Return the last page. | ||
yield page; |
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.
You previously had return here and I looked up why. It not only yields the last page but marks the iterator as done so I thought you were right. Why do you think it should be yield?
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.
That's what I initially expected too, and that's why I had a return, but in tests that didn't work as I expected. If I understood correctly, the iterator has a return
function to mark it done, but here the function is a generator, which returns said iterator, hence the difference. None of the generator examples in the MDN docs use return, and here is an example in the Node CLI:
> function* test() {
... yield 1;
... yield 2;
... yield 3;
... return 4;
... }
undefined
> const i = test()
undefined
> Array.from(i)
[ 1, 2, 3 ]
This adds a helper function to make it easier to go through all of the paginated results. It allows syntax like the following:
Unit tests aren't completely thorough, because technically they should check for all the error conditions, which are already checked in the underlying
query
function. I don't think it's worth the overhead.