-
-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
API: Pagination with offset instead of page #5093
Comments
The question is, if we replace the |
@RaoHai it's relatively easy math to get from one to the other :) We only need to replace the page concept in the Posts API, not on the frontend of the blog, currently the maths for getting from page to offset is hidden away in the depths of the model. Removing this abstraction out from the model layer makes sense to me. Further, when we add an API for Channels that will probably have a page option, rather than offset and will abstract the maths behind a reusable interface for figuring out all manner of details to do with a specific channel including next & previous pages. |
Closes TryGhost#5152, TryGhost#5093, TryGhost#5151 - Adds `featured` filter option to posts.browse method modifying the model to take it too - Implements offset pagination method to posts.browse method modifying the model to work with it too but still supporting the `page` parameter - Removes `staticPages` parameter in options filter to allow using the `page` parameter with options `all` `true` or `false
Question on this one, the findPage method right now is like: /**
* #### findPage
* Find results by page - returns an object containing the
* information about the request (page, limit), along with the
* info needed for pagination (pages, total).
*
* **response:**
*
* {
* posts: [
* {...}, {...}, {...}
* ],
* page: __,
* limit: __,
* pages: __,
* total: __
* }
*
* @param {Object} options
*/ After solving this should look like: /**
* #### findPage
* Find results by page - returns an object containing the
* information about the request (offset, limit), along with the
* info needed for pagination (total).
*
* **response:**
*
* {
* posts: [
* {...}, {...}, {...}
* ],
* offset: __,
* limit: __,
* total: __
* }
*
* @param {Object} options
*/ is that right? |
Sorry I was convinced I already replied to this - the answer is yes, the input and output should be offset rather than page :) |
I've cleaned up the API layer such that pagination only exists in one location now: |
- Removing the page parameter in API operations - Adding the offset parameter in API operations - Fixing the tests for the new concept
I've been mentally flip-flopping on this particular change for a while. Without it, there are some limitations to how the API can be paginated. The case of wanting a different number of items in the first set to the second set, E.g. homepage shows 1 post, subsequent pages show 20 posts, doesn't work because if you change limit between page 1 and page 2 you'll either miss out or duplicate some items because the offset cannot be correctly calculated. However, wholesale switching from using Additionally, we still have the idea that our API probably needs to move back towards conforming to JSON API (although this is still being considered). JSON API's position on pagination is very specific:
It further specifies that any pagination strategy can be used, by using keyed parameter names like Given that our API is going to move all filter keys down under a filter parameter - e.g. if you want to filter based on the Therefore I propose that we keep At the moment we return the following, all as integers:
JSON API specifies that a response should return Long term, I think we could add a Main question is, anyone have any reason why we can't punt this to deal with more fully later? |
All seems like sound reasoning 👍 My main concern is about the public API - if we're definitely intending to add |
I had the same thought, and figured that making this change now 'just in case' we make another change later doesn't really make sense. If we were 100% that this is what we were going to do then I'd be in favour, but as it doesn't match what any other params look like it seems weird. Also, in future if we do go down this road, we can output deprecation warnings for |
👍 |
I posted about this on the json-api Github (json-api/json-api#1163) but since Ghost uses offset pagination, I'm curious:
|
This is needed in order to fully support channel customisations, see #5091
As part of the move towards refactoring the Ghost frontend into a fully customisable and extensible set of channels, one of the features or customisations we want to make available is the ability to set a different number of posts-per-page for the first page vs subsequent pages in a channel.
E.g. make it so that the homepage shows 1 post, and every subsequent page in the index channel shows 10.
At the moment, pagination in the API is managed by passing in a
limit
(how many posts per page) and apage
number. This enforces that every page in the set must be the same size. This limitation can be overcome by telling the API alimit
(how many posts you want) and anoffset
(where in the list to start from). Calculations around the required offset for a given page can then be done based on the channel settings.Therefore, we should replace the
page
concept in the API with anoffset
concept instead, for all of our findPage methods.See also #2896
The text was updated successfully, but these errors were encountered: