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

Respect Cache Header #11

Closed
hottehead opened this issue Jan 28, 2020 · 3 comments
Closed

Respect Cache Header #11

hottehead opened this issue Jan 28, 2020 · 3 comments

Comments

@hottehead
Copy link

I scanned the docs and the code and I saw that the cache plugin does not respect cache header Expires and Cache-Control which may be sent from the upstream.

Is this correct? Is this on purpose?
If not I would implement this and create a PR.

@jpodwys
Copy link
Owner

jpodwys commented Jan 29, 2020

You're correct, this repo does not respect cache headers. I'm open to supporting this (thank you for being willing to open a PR).

There is already a fork of this repo that adds cache header support. They submitted a PR in early 2018. At the time, I was weary of adding third-party libraries directly to this repo, so I raised that concern.

I'm now willing to reconsider merging their changes into this repo. It looks like they're currently 21 commits ahead and 6 commits behind, so there may be some conflicts, but probably nothing big.

Please have a look at that fork and let me know if it would meet your needs.

@hottehead
Copy link
Author

I checked the PR. This meets my needs. I think the solution is well done and should be merged.

How about reopening the PR? The changes seem valid as is. And there are no more required changes on master that are not on the feature branch.

Using https://github.com/kornelski/http-cache-semantics#readme package seems valid even though it's third party, considering the complexity of cache policies.

@jpodwys
Copy link
Owner

jpodwys commented Feb 3, 2020

Thank you for your thoughts! I'll reopen the PR and reach out to the fork owners.

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

No branches or pull requests

2 participants