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

Implement iteration API, in addition to pagination API #235

Merged
merged 13 commits into from
Nov 17, 2021
Merged

Conversation

Pimm
Copy link
Collaborator

@Pimm Pimm commented Nov 12, 2021

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.

mollieClient.payments.page()
.then(payments => {
	// Apply logic.
	return payments.nextPage();
})
.then(payments => {
	// Apply logic.
	return payments.nextPage();
});

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:

for await (let payment of mollieClient.payments.iterate()) {
	// Apply logic. Use the break keyword when you're done.
}

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:

// Find the 10 latest euro payments over €100.00.
const iterator = mollieClient.payments.iterate()
	.filter(({ amount }) => amount.currency == 'EUR' && parseFloat(amount.value) > 100)
	.take(10);
for await (let payment of iterator) {
	// Apply logic.
}
// Find a payment by description.
const payment = await mollieClient.payments.iterate()
	.find(({ description }) => description == 'Transaction #b0e547a7');
// List all Italian customers.
const iterator = mollieClient.customers.iterate()
	.filter(({ locale }) => locale == 'it_IT')
for await (let customer of iterator) {
	// Apply logic.
}

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 write mollieClient.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.

@Pimm Pimm force-pushed the pimm/v3.6.0 branch 9 times, most recently from 58254f7 to 5c51b94 Compare November 16, 2021 10:53
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.
Copy link
Collaborator

@edorivai edorivai left a 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?

@Pimm
Copy link
Collaborator Author

Pimm commented Nov 16, 2021

Thanks, @edorivai. iteration/iteration.test.ts is essentially a test for HelpfulIterator. It doesn't test it directly like a unit test would, but it does cover all of the methods (forEach, filter, et cetera) except for map ‒ which TransformingNetworkClient uses and is therefore automatically covered.

What could be a good idea, though, is to add a test which iterates over several pages. iteration/iteration.test.ts only covers a handful of payments, so they're all on the same page.

@edorivai
Copy link
Collaborator

Oh yeah, that makes sense. These tests should cover it then

@Pimm Pimm merged commit 39679f2 into master Nov 17, 2021
@Pimm Pimm deleted the pimm/v3.6.0 branch November 17, 2021 08:34
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.

2 participants