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

This should work - once vivek comes back let's discuss. #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

nmondal
Copy link

@nmondal nmondal commented Dec 31, 2020

Response cachedResponse = new Response.Builder()
.request(original)
.protocol(Protocol.HTTP_1_1)
.code(201)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why 201?

Copy link
Author

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 ){
Copy link
Contributor

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).

Copy link
Contributor

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.

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?

Copy link
Contributor

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.

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

Successfully merging this pull request may close these issues.

None yet

3 participants