Respect pagination in helpers

[Pimm]

The following helper functions return a (plain) array, although the underlying endpoint is paginated:

• customer.getMandates,
• order.getRefunds,
• payment.getChargebacks,
• payment.getRefunds,
• profile.getChargebacks,
• profile.getPayments,
• profile.getRefunds, and
• subscription.getPayments.

This means the lists returned by these functions may not be exhaustive, which may surprise developers.

For some functions ‒ such as customer.getMandates or order.getRefunds ‒ this is realistically not an issue. It is unlikely that a single customers has enough mandates for the API to start paginating them, or for a single order to have enough refunds for the API to start paginating them. However, for profile.getPayments, the scenario is more likely.

I see three possible solutions:

Introduce nextPage

We could (very easily!) add the nextPage function to the returned lists. This will allow developers to do:

var payments = await profile.getPayments();
payments = await payments.nextPage();

However, developers may not notice the existence of the nextPage function. And if they don't, we're back to square one. We could additionally rename profile.getPayments to profile.pagePayments to convey the paginated nature.

It would be an easy solution, but a breaking change. And in my opinion, not a pretty API.

Make the arrays exhaustive

We could repeatedly call the endpoint until the list is depleted. The returned arrays would always be exhaustive. This would not cause a breaking change. However, the helper functions would become potentially expensive and might put a lot of stress on the Mollie API.

Have the helper functions return iterators

Developers would have to do:

for await (let payment of profile.getPayments()) {
  […]
}

instead of:

for (let payment of await profile.getPayments()) {
  […]
}

It would be a breaking change as well.

The only other downside to this solution I can see, is that ‒ as some other helper functions rely on endpoints which are not paginated ‒ the API would be inconsistent. profile.getPayments would return an iterator, while profile.getMethods would return an array. (We could consider returning iterators from all list-y helper functions.)

[edorivai]

My knee-jerk reaction would be to consistently have the helper function return iterators. This should feel familiar for consumers that have been using the .iterate() methods. It's also the best fit with the current API IMO.

Returning iterators from all list-y helpers sounds like a good option. Not sure how confusing it would be that profile.getMethods() would return an array though :thinking:. One could still:

for (let method of await profile.getMethods()) {
  […]
}

right?

[Pimm]

I guess that's the decision we'll have to make.

If profile.getPayments() is going to return an iterator, does that mean we want profile.getMethods to return an iterator as well? Returning an iterator everywhere would be consistent; returning a (promise of an) array where we can would be "friendlier". Arrays are better understood in the JavaScript community, so there is an argument to be made for returning arrays where possible.

[edorivai]

Oh I meant it the other way around. If we return an Array, the for (let ... of) syntax will still work. So that might potentially make it less confusing to be returning an array there. The confusion I would foresee is people get into the habit of using for ... of syntax to loop over objects returned from the Mollie API. That would still work, even with arrays!

So I guess my question is: what objections do we really have against returning arrays from endpoints that aren't paginated in the first place?

[Pimm]

Hmmm…

If profile.getMethods returns an array, then this will work:

for (let method of await profile.getMethods()) {
  […]
}

If it returns an iterator instead, then the code must be changed to this:

for await (let method of profile.getMethods()) {
  […]
}

On arrays, we can use for…of; but on (async) iterators, we must switch to for await…of.

We see a similar effect when using forEach. If profile.getMethods returns an array, then this will work:

(await profile.getMethods()).forEach([…])

If it returns an iterator instead, then the code will still kind-of work. The await keyword will have no effect, because the iterator itself is not thenable (not a promise). The code above would be equivalent to simply:

profile.getMethods().forEach([…])

But likely, you will want to wait for the loop to complete. In which case the code will have to be changed to:

await profile.getMethods().forEach([…])

There are subtle differences depending on whether we return a (promise of an) array, or an (async) iterator.

[Pimm]

We could return arrays which also implement the async iterator interface. Am I overcomplicating things now?

[edorivai]

We could return arrays which also implement the async iterator interface. Am I overcomplicating things now?

Yes I think this would overcomplicate things.

Yeah this is a tough call. I definitely prefer plain arrays over iterators if we have all the data available. But that doesn't work for paginated endoints, so we are forced to use iterators there (or some other async construct, but of the async constructs I do like iterators here).

Maybe we should optimize for a solution which would be easiest to change over if we decide we want to go a different route later on. :thinking:

[Pimm]

Maybe we should optimize for a solution which would be easiest to change over if we decide we want to go a different route later on. 🤔

Switching from an (async) iterator to a (promise of an) array or vice versa will always be a breaking change. But perhaps those are not the only two options. I might have tunnel vision here.

Now that I've slept on it, I'm personally in favour of arrays where possible. For two reasons:

• Returning an array where possible makes the API more honest. Returning an iterator hints that there's some kind of streaming going on, which there isn't.
• The API already has iterate methods which return iterators; list methods which return arrays (and page methods which return arrays with a nextPage function attached). In other words, when you use this client, you're by definition dealing with a mix of data structures depending on whether you're using paginated endpoints or not. Mixing data structures in our helpers may not be as surprising as we think it is.

[janpaepke]

Intuitively I was also drawn to the "let's make it consistent" approach.
In this scenario we'd be forced to go with the common denominator: you CAN iterate over both arrays and iterators (albeit with subtle differences) but you CAN'T stream arrays. So initially I was in favour of turning everything into iterators.

After thinking and reading about it more, I changed my mind though.

I don't think the inconsistency is bad here. On the contrary I think it verifies the premise of this change in the first place:

This means the lists returned by these functions may not be exhaustive, which may surprise developers.

So this means when an endpoint returns an exhaustive list it should be an array, when it is paginated it should return an iterator. This both makes intuitive sense to a developer, as well as clarifies intent and deliberation.

It also means that if at some point an endpoint changes from being exhaustive to being paginated (array -> iterator), it's a breaking change, which forces a developer to consider his implementation. After all using for await…of instead of for…of is likely not the only thing that needs to be considered in this scenario.