Feature | V2 Paginator

[Sammyjo20]

No description provided.

[what-the-diff]

• Added a new method to the ConnectorContract, which returns an instance of RequestPaginator.
• Created two interfaces: RequestPaginator and SerialisableRequestPaginator (which extends JsonSerializable).
• Implemented HasRequestPaginator in Http\Connector, so that it can be used by any connector implementing this class or interface(s) extending from them.
  4a-b/5a-c/6d: The actual pagination logic is implemented in OffsetRequestIterator; PageRequestIterator will follow soon™️!
• Added a new class PageRequestPaginator, which extends RequestPaginator.
• The constructor of the base class now accepts an optional limit parameter (defaults to null).
• Both classes are serialisable and unserialisable via __serialize() and __unserialize().

[Sammyjo20]

Looking really good so far

[Sammyjo20]

Also if you want to try it against Saloon's test API here are two endpoints:

per-page/page pagination: https://tests.saloon.dev/api/superheroes/per-page
offset-limit pagination: https://tests.saloon.dev/api/superheroes/limit-offset

[juse-less]

I'll fix the failing tests when I move 'resolving' parts out of PageRequestPaginator and OffsetRequestPaginator and into the test implementations.

I still have a lot of stuff to figure out. I think each of them should have an 'attached' TODO next to them.

[juse-less]

Also if you want to try it against Saloon's test API here are two endpoints:

per-page/page pagination: https://tests.saloon.dev/api/superheroes/per-page offset-limit pagination: https://tests.saloon.dev/api/superheroes/limit-offset

@Sammyjo20 Technically speaking, 'paged' APIs should really always have 1 as the first page.
But noticed that the per-page endpoint doesn't return any first page - only last.
I'm not sure if it makes sense to add it for consistency, even though it'll always be 1?

[juse-less]

Regarding serialising the RequestPaginators, I think it's better to add that as a feature request after v2 has landed.
That one could take quite a bit of time to figure out.

I believe the only blocker, technically speaking, are middlewares as they can be Closures, that aren't really serialisable, AFAIK.
There are packages to make it easier, but fairly certain it doesn't always work.

Request headers, query parameters, body payload, and configuration should all be easily serialisable.

Same thing with the Connector. The only caveat is if people add things like credentials as arguments.
But considering both the Request and Connector should serialise themselves, it's more of a documentation thing for implementors to add those properties to the __serialisation() and __unserialisation() methods.
So not really a problem.

---

Maybe we could ask how much it'd be used, as well.
If it wouldn't really be used, then we can drop the idea so we don't need to maintain something no one would use, as it feels kinda complex to implement.

Context:
We've been talking about making RequestPaginators serialisable so that they can be sent into things like Laravels background Jobs.
This makes it possible to iterate, say, 25 requests, and pass the iterators to another Job and continue there, automagically.
In order to retain the same state, we need to rebuild the same Connector and Request, so that they send the very same data, apart from whatever is changed per request.
So it's not as easy as passing on current page, or so, as Request or Connector could change between Jobs.

[Sammyjo20]

I think it's better to add that as a feature request after v2 has landed

Yeah I actually agree on this one. We firstly have the issue as you mentioned with middleware and while I don't mind telling people "if you want to serialize the paginator, you have to switch to invokable class middleware" I don't actually know how many people would really use it. The only time I'd use it, would be if I was rate limited by the API - I would want to queue a job with the backoff time and then run the process again.

That being said, this would actually solve a really annoying issue we have at work where an API only gets so far and then stops at exactly the same point because it gets rate limited. Did you figure out a way to "resume" the iteration process from where it was left off? I wonder if we set the page in the "rewind" method to be the unserialized last page, so it starts at page 10 for example. What we also could do is when we know we are on the last page, we reset the "starting point" back to zero so if someone starts iterating over it again, the rewind doesn't start at page 10 again. 💡

I kind of think it'll be worth it, especially when all they have to do is migrate middleware to be invokable classes which are cleaner typically. If we can implement it later on in v2 hopefully without any breaking changes then happy days, but I think what you have come up with will be great for now.

[juse-less]

Did you figure out a way to "resume" the iteration process from where it was left off?

Yeah - I've solved the iteration/paging continuation.
Have a look at the the rewind() method.
See this DocBlock in the contract for more info.
By default, we'll continue where it left off. But can be disabled.

If we want to continue in a separate process, we'd need to be able to serialise- and unserialise it to retain the same state.

If we can implement it later on in v2 hopefully without any breaking changes then happy days, but I think what you have come up with will be great for now.

Currently, the serialisation bits are actually a separate interface.
So what we'd do is remove it from this feature branch, and add it later, with a trait, or so, that can be added to custom Paginators for easy serialisation.
So I'm pretty sure that we'd be able to add the serialisation in a separate feature PR, without breaking changes.

But let me have a think next week, and maybe I can solve it with non-Closure middlewares.

[juse-less]

I tested a bit, and it looks like what I originally had is actually correct.
I removed the iterable stuff myself because PhpStorm was complaining. But it looks like a bug in PhpStorm, now that I tested a bit more.

See my recording of a quick test

ScreenShot.2023-02-15.at.03.03.14.mp4

So to summarise - for the $requests, including Pool, Connector, etc, the type hint should be

* @param iterable<\GuzzleHttp\Promise\PromiseInterface|\Saloon\Contracts\Request>|callable(\Saloon\Contracts\Connector): iterable<\GuzzleHttp\Promise\PromiseInterface|\Saloon\Contracts\Request> $requests

I guess it makes sense to specify its keys, as well. Kinda redundant, but sometimes PHPStan wants them when giving iterables to methods or classes using templates.
So that'd leave us at.

* @param iterable<array-key, \GuzzleHttp\Promise\PromiseInterface|\Saloon\Contracts\Request>|callable(\Saloon\Contracts\Connector): iterable<array-key, \GuzzleHttp\Promise\PromiseInterface|\Saloon\Contracts\Request> $requests

[Sammyjo20]

Self review

[Sammyjo20]

Self review

[Sammyjo20]

We're finally good to go! Woohooo! Thanks @juse-less for the help with this <3