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

ETag is not implemented properly #31

Closed
jsardev opened this issue Oct 24, 2019 · 3 comments · Fixed by #32
Closed

ETag is not implemented properly #31

jsardev opened this issue Oct 24, 2019 · 3 comments · Fixed by #32

Comments

@jsardev
Copy link

jsardev commented Oct 24, 2019

I think that this library has some wrong assumptions on the ETag header. As I see the code, the ETag is being set on the response, but it's never read afterwards (on the request).

The library should look at the If-None-Match header and compare it to the cached response ETag and return 304 Not Modified when they're the same. Now, the library always sends data after the response expired which is a waste.

Example

@jsardev jsardev changed the title ETag is not being used ETag is not implemented properly Oct 24, 2019
@Kikobeats
Copy link
Owner

You're right, without If-None-Match validation then the process is not complete.

My concern right now is, is it worth to?

Why not remove it and just use cache-control?

@jsardev
Copy link
Author

jsardev commented Oct 25, 2019

@Kikobeats By just using Cache-Control, your server will always send data on the next request after cache expiry. By implementing ETag + If-None-Match, the server will not send any data, just return 304, and the browser will understand that the cache is still valid. This is an improvement i.e. for mobile devices, which should save as much data on the network as possible.

If you don't mind I'd like to propose a PR with this 😄

@Kikobeats
Copy link
Owner

I'll be a pleasure to accept a PR about this 🙏

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 a pull request may close this issue.

2 participants