-
Notifications
You must be signed in to change notification settings - Fork 3
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
This should work - once vivek comes back let's discuss. #6
base: master
Are you sure you want to change the base?
Conversation
Response cachedResponse = new Response.Builder() | ||
.request(original) | ||
.protocol(Protocol.HTTP_1_1) | ||
.code(201) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why 201?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copied from original code : CREATED 201
:-)
.message("") | ||
.body(ResponseBody.create(responseJSON, okhttp3.MediaType.parse(MEDIA_TYPE_JSON))) | ||
.build(); | ||
if ( HEAD_PEEK ){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do this without compromising on the time taken to load the response. Since we use observables in android, we could sent the cached response as is and at the same time send a parallel request in a co-routine to check the validity of this response (using the head message like below). This way the UI will load faster and at the same time, load the changed response (if server has updated response).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Murtaza-arif , @sarthak-baiswar - who wants to take this up? @nmondal has created a partial poc with this pull request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But for every GET api, we need to create a HEAD API as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just the idea. We have to see if apiv3 supports adding custom headers, if not it will be handled as an additional attribute in sirius response. The idea is correct - the implementation is left to you guys on what integrates better with compass.
https://compass-tech.atlassian.net/browse/AAAA-2514