Add support for paginating results.

[n1k0]

This highly WiP patch intends to bring support for paginating results to the API.

Most of reformatted code is because I've upgraded my elm-format installation to the latest version available 😑

[n1k0]

Ok, I think I have a first version ready for review here. I think the patch could still be improved with Kinto.Pager being able to act as an higher-level accumulator so we could just pass it around, though I'm not sure, so I'd like to get feedback from @magopian and @glasserc. Thanks!

[glasserc]

It looks OK to me but a little barebones (which also means simple). Strings are easy to misuse. I thought it might be good to have the Pager store a Cmd that you could invoke to get the next page, or a (Result -> Cmd) -> Cmd, but I recall reading somewhere that storing functions in models is bad in Elm for some reason?

Is it possible to maybe make the Pager store an HttpBuilder or something instead? I guess that's not much better because the user might think they're expected to send the request themselves or something.

I guess one easy thing to do might be to store the "next page" token as an opaque type. Not sure it's really worth it..

[n1k0]

It looks OK to me but a little barebones (which also means simple).

Sounds good to me heh

Strings are easy to misuse.

Well in the case of a URL we can't do much about that. The good thing being that if it's invalid in any way, the request will fail and we already handle HTTP errors.

I thought it might be good to have the Pager store a Cmd that you could invoke to get the next page, or a (Result -> Cmd) -> Cmd, but I recall reading somewhere that storing functions in models is bad in Elm for some reason?

I have no problem storing functions in a model (actually when you store a type, it's a function). I'm slightly uncomfortable storing a single Cmd in a pager as one might want to use different ones to deal with a single pager instance, though I can't really see a good use case for that yet. But I usually tend to decouple things as much as I can, eg. data from data processors.

Is it possible to maybe make the Pager store an HttpBuilder or something instead? I guess that's not much better because the user might think they're expected to send the request themselves or something.

Yeah, sounds complex for no great added value imho.

I guess one easy thing to do might be to store the "next page" token as an opaque type. Not sure it's really worth it..

That would mean rebuilding the next page full URL using the base server and resource endpoint accordingly, which sounds... quite a bit overengineered to me.

[n1k0]

While working on this patch, I found useful to also expose the Total-Records http header (https://github.com/Kinto/elm-kinto/pull/27/commits/f36cfc189a271e73931c9b23b60ac923e2c76bbc).

[n1k0]

Woops, missed your review, I've just posted a large update to turn a Pager into an accumulator in https://github.com/Kinto/elm-kinto/pull/27/commits/69eb88851090c63c3dc98f6ea1151579f2468c6c :(

I'm sorry, most of your comments are now outdated, but I think it's a little better. (Edit: no, actually only one of your comments was outdated)

[n1k0]

I think I'd like the ability to still requests non-limited lists (and thus get the list back instead of a pager). Would it make sense to have a getList and a getPaginatedList (which would include the limit that we could previously add to the list of headers manually)?

That's a very good idea indeed. The more you can!

I also wonder if the Pager type could "contain" everything needed to retrieve the following results. I mean that it could maybe contain the resource decoder, the client... everything, so the user could save the pager in their model, and then call something like Kinto.getNext model.pager?

That's basically what I did in https://github.com/Kinto/elm-kinto/pull/27/commits/69eb88851090c63c3dc98f6ea1151579f2468c6c... Great minds!

[n1k0]

I've restored getList as its previous version and added getPaginatedList as you suggested in https://github.com/Kinto/elm-kinto/pull/27/commits/e06adcdd3d552486c45ba91db7540232ec3d5883

[n1k0]

@magopian addressed your comments in https://github.com/Kinto/elm-kinto/pull/27/commits/d387d1154cf3f9bda072d40076b0ff576961e6e7

I didn't remove the limit helper as it felt a little awkward making it a mandatory arg to getList, but we may revisit this.

[magopian]

Well, it seems this is ready to land now, what do you think?

Awesome work @n1k0 , thanks! Do you also want to release a new major version?

[n1k0]

I'd be ok with landing this and make a new major release. I'm still unsure about enforcing a limit arg to getList. On one hand I'd find it very explicit, on the other hand limiting if you want to rely on the server configured limit.

I'll make my mind tomorrow I guess.

[n1k0]

There's also something with embedding the client into a Pager; if you look at the modified example app code, I initialize pager in init with pager = Kinto.emptyPager client recordResource; that works fine when you have a static client definition like in this app, but what if someone wants to configure a client after the user has entered some information?

Of course, then pager in the Model could just be Maybe Pager, but that would mean pattern matching a maybe, which is always a little cumbersome. I really liked we could initialize the model with just an empty pager.

[n1k0]

I've updated our example app to have the client configurable at runtime in https://github.com/Kinto/elm-kinto/pull/27/commits/050a362a80cb253955c97076e956607b8c293d17.

Spoiler: this Maybe Client and the inevitable Maybe Pager we have in the model makes apps like this really painful to write. While Maybe Client is totally fair to have as it denotes if it's been configured just yet, Maybe Pager seems really awkardly verbose to handle, and a little flaky tbh.

[n1k0]

Travis fails with this error:

/home/travis/build/Kinto/elm-kinto/examples/tests/Main.elm has an invalid module declaration. Check the first line of the file and make sure it has a valid module declaration there!

It's because elm-test has released a patch version which actually breaks the API haha. I dealt with that for tooty yesterday, will fix.

[magopian]

I'm a bit lost, what don't you like about the current state of the code? And what would you rather do?

To me, the current version looks good to me, appart maybe from what we've discussed about the record list being handled by the pager itself. I'm still not sure I'm comfortable with that, but I understand your arguments, saying that it's way more usable and ergonomic like that.

[n1k0]

I'm a bit lost, what don't you like about the current state of the code?

The fact that we have to deal with so many Maybes.

And what would you rather do?

I don't have any good solution in mind, unfortunately.

appart maybe from what we've discussed about the record list being handled by the pager itself.

Did you look at the final diff? that's not the case anymore as I'm now providing an updatePager for people wanting to accumulate objects in the pager. So it's simple AND explicit, which is a good compromise to me.

[magopian]

I read https://github.com/Kinto/elm-kinto/pull/27/files#diff-c3fd477279f609c65aaadbc0a669aec6R307 , the objects is still in the pager, right? It's not an issue though, as I explained in my previous comment. It does feel pragmatic ;)

Regarding the "too many Maybes", what would you recommend? I'm afraid this is just the way it has to be in this configuration, or is there another solution?

[n1k0]

the objects is still in the pager, right?

That's exactly my point, we now have two clients, the one for the app and the one embedded in a pager. And a pager can't be the single source for a client, as it only deals with lists and pagination. Hence why I'm stuck. Maybe we need another abstract structure? Which one, any idea?

[n1k0]

the objects is still in the pager, right?

Ah, the objects, so you mean the list of objects, not the client. So yeah, the list is still in the Pager. I'm lost as fuck, where would you want to put this list?

[n1k0]

Released in 4.0.0.