-
Notifications
You must be signed in to change notification settings - Fork 64
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
Implement iteration API, in addition to pagination API #235
Conversation
58254f7
to
5c51b94
Compare
We're still debating over the exact API for iteration. This commit serves mostly as a proof of concept.
Nock monkey patches http.request, which interferes with the Nockless tests. Having the Nockful tests in their own project (with the runner hack) ensures they run in their own worker.
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.
Only comment I have is that it might be a good idea to write some unit tests for the HelpfulIterator
, what do you think?
Thanks, @edorivai. What could be a good idea, though, is to add a test which iterates over several pages. |
Oh yeah, that makes sense. These tests should cover it then |
Rationale
The Mollie API provides paginated endpoints, which are great if a developer using this library is implementing a paginated dashboard.
Some developers ‒ myself included ‒ (ab)use these paginated endpoints and the
nextPage
method to apply logic to a batch of values in a set which don't necessarily fit on one page.This is inconvenient to the developer, because the
nextPage
pattern above requires a lot of boilerplate code; and depending on the exact code might be inefficient in terms of memory usage as well as load on the Mollie API.Basic
This pull requests implements an API which lives alongside the paginated API, and is designed for this case:
Advanced
There is a (currently stage 2) proposal to add helpers to iterators. In anticipation of the inclusion of these helpers, some of them are polyfilled in our library.
This powers the following snippets:
With a bit of luck, we can discard our polyfill if and when the proposal is accepted and lands in Node.js without any breaking changes.
Note that not the entire proposal is currently supported by the polyfill. Specifically (and deliberately),
toArray
is not included.toArray
would allow developers to writemollieClient.payments.iterate().toArray()
, which is super convenient, but also potentially puts a lot of stress on the Mollie API (which will return all payments ever made) as well as the server of the merchant (which will have to deserialise and store all of those payments).Future
If the Mollie API starts to support some of the cases above natively ‒ without iteration, we might be able to expand the logic to use that to our advantage and improve efficiency.
Discussion
Mollie has experienced incidents where developers were making so many requests in rapid succession that the API was having performance and reliability issues.
Both before and after this PR was merged, we've discussed whether the introduction of the iteration API makes it too easy or too tempting to make an unreasonable amount of requests ‒ essentially performing an accidental
DDoS attack on Mollie ‒ compared to the pagination API.