Replace Pagy with Geared Pagination #445
Conversation
This is a simpler alternative, and also enables endless pagination, which is also implemented in this PR.
There was a problem hiding this comment.
I love the infinite scroll! ⚙️
My one feedback is that there are a handful of breaking changes. This isn't a massive deal because this API is lightly used, however, I would encourage trying to match the old API spec.
If that's not possible, then there are a couple of things we should consider:
- Let's make sure the API wrapper from the other repo still works
https://github.com/hackclub/hackathons/blob/main/lib/data.js - We should consider creating a V2 version because this is a breaking change
- incrementing the API version and support both V1 and V2
- deprecate V1
- and then completely remove V1 after a month
| scaffold_url.gsub("__pagy_page__", page.to_s) | ||
| unless @page.last? | ||
| json.links do | ||
| json.next url_for(page: @page.next_param) |
There was a problem hiding this comment.
It looks like this is a breaking change because we're losing the keys first, prev, last, and self.
Could we implement those links so that we don't have a breaking change?
There was a problem hiding this comment.
First seems redundant since it's the initial page, and self too for obvious reasons. I tried to implement previous and last, but the library doesn't easily support it and I don't see any link but next being beneficial since all clients would ever need to get while iterating through pages is a link to the next one. Regarding a version increment, from what I can tell there was never an expectation of stability for external use, especially for something so minute that we didn't even use, but we should be able to do that pretty easily so it's not a huge blocker.
| @@ -35,12 +24,6 @@ def shape_for(record, json) | |||
| yield if block_given? | |||
|
|
|||
| json.created_at record.created_at if record.respond_to?(:created_at) | |||
|
|
|||
| json.links do | |||
There was a problem hiding this comment.
Comment above about breaking change applies here too
| included do | ||
| include Pagy::Backend | ||
| helper_method :pagy_metadata | ||
| after_action { pagy_headers_merge(@pagy) if @pagy } |
There was a problem hiding this comment.
I think losing the headers would be another breaking change
There was a problem hiding this comment.
This is a much simpler alternative, and also enables endless pagination, which is implemented in this PR.