Skip to content
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

Paged Data Format Update #34

Open
HankG opened this issue Dec 8, 2018 · 3 comments
Open

Paged Data Format Update #34

HankG opened this issue Dec 8, 2018 · 3 comments

Comments

@HankG
Copy link
Contributor

HankG commented Dec 8, 2018

I believe what the original design was trying to go for was the implementation of RFC 5988, the way GitHub ultimately decided to go (as described here). While it's a standard I think this will make things more difficult on the receiver side. The interim implementation I have now is a more traditional implementation where endpoints with paging will return a two-part response that looks like:

{
   links:  {
        //standard paging keywords like below
        previous: // URL to previous,
        next: // URL to previous
   }
   data: //whatever the return data format is
}

I'm thinking I'm going to go back and see how much more difficult the RFC versus over a traditional method would be in languages that would most likely have clients written in them: Java/Kotlin (for Android), Swift/Objective-C (iOS/macOS), and JavaScript. I'm sure it is possible but it's a question of cumbersomeness in cracking open the HTTP header, support for proper parsing and field handling, etc. The way the code architected switching to the RFC version shouldn't be difficult.

@denschub
Copy link
Member

I personally absolutely dislike having non-content in the response body. Pagination details are not part of the API payload itself, but related to the request flow, which is kinda why it makes sense to have that in the HTTP headers.

If we find more support (@SuperTux88 maybe?) for your proposal, that's fine.

@denschub denschub added this to the API Version 1 milestone Jan 26, 2019
@HankG
Copy link
Contributor Author

HankG commented Jan 26, 2019

I personally absolutely dislike having non-content in the response body. Pagination details are not part of the API payload itself, but related to the request flow, which is kinda why it makes sense to have that in the HTTP headers.

OK. FYI my suggestion above is how it is implemented right now. It can of course be changed. You two generally move in lockstep so I have a feeling I will be on the losing end of this vote but I'll wait and see what comes back before embarking on any changes.

@denschub
Copy link
Member

The main reason I am unsure if in-body pagination is a good idea is some feedback I gathered from people working on larger-scale social networking mobile application thingies. They explained it's quite common to separate response parsing and body parsing into two threads, because there is no reason to block the networking thread with a task like parsing JSON. However, they need the pagination information in the networking thread, in order to fire follow-up requests on larger requests ("the user came back after one day, let's load all the post since then while they are scrolling to the top of the list"). If they'd end up with pagination URLs in the body, they'd need to create a channel back to the network thread, which can be quite an issue.

In addition to that, not having the pagination in the body makes the data passing between the JSON encoder and the view module rendering a list much more reusable, because you can just cache the entire body and have the view always render the full JSON, and just ask their parent for the next set of items. The parent can then decide whether to load from cache or from a network resource. If you have the pagination URLs in the body, the parsing/drawing module actually has to make the decision between "ask the network module to load this URL" and "ask the cache for the next set"; or pass along the entire body to a decision-making module.

But that's probably implementation-pr0n, and may not even be relevant for us, hence me asking for additional voices.

You two generally move in lockstep

Besides the fact that we disagree quite regularly, which is the reason I pinged him in the first place, I asked for any additional support. This is an open repository, and if anyone favors having the pagination links in-body instead of in the header, I see no reason to not do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants