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 Server response Cache related headers. #6

Conversation

valdemon
Copy link

@valdemon valdemon commented Aug 1, 2017

Kozba, Waldek - Externer Mitarbeiter added 8 commits August 2, 2017 00:23
- adopted tests
- additional test cases
- support for client request cache directive headers
- README.md updated
- added 'bypassHeaders' to 'pruneHeader' automatically,
- test case for the 'bypassHeaders'.
- returning cloned response from a cache.
- optimized 304 handling.
@jpodwys
Copy link
Owner

jpodwys commented Aug 14, 2017

Hi thank you for this PR! I apologize for my late response. When I wrote this library, I had two primary goals for code maintenance:

  1. Keep the download size as small as possible
  2. Keep the barrier to entry as low as possible

While this PR adds important convenience, it also 1) significantly increases the download size and 2) increases the barrier to entry by requiring that a transpiler be used for browsers lacking ES6 support.

If I were to merge this PR, I would need to cut a breaking release due to the new requirement for a transpilier (cutting a breaking release does not bother me). However, before talking more about merging this PR, I'd like to discuss the possibility of making this PR into either a superagent plugin that coexists with superagent-cache-plugin, or perhaps a plugin that can be used to directly modify superagent-cache-plugin. I know that sounds odd, but it would give users the freedom to choose whether they want to download the extra code at all--I don't like the idea of forcing all users to download code they're unlikely to use unless it's only a marginal increase in download size.

What are your thoughts?

@jpodwys
Copy link
Owner

jpodwys commented Apr 23, 2018

Closing due to lack of response.

@jpodwys jpodwys closed this Apr 23, 2018
@jpodwys jpodwys mentioned this pull request Jan 29, 2020
@jpodwys jpodwys reopened this Feb 3, 2020
@jpodwys
Copy link
Owner

jpodwys commented Feb 3, 2020

@valdemon At the time you originally submitted this PR I was overly hesitant to add third-party libraries. I was wrong in that regard. You've done excellent work here and I'd like to merge your fork. I'll go ahead and work on resolving merge conflicts. Please let me know if you have any thoughts.

A number of people have requested this feature, the latest of which can be found here.

@valdemon
Copy link
Author

valdemon commented Feb 3, 2020

Hi @jpodwys cool to hear that!
I'm not with Finanzcheck.de any more though, what means I've lost the direct control over the PR branch.
Please go ahead and if you'd have any issues with the PR just comment here. It's been a while since I committed this but at least I can try to support you ;)

@hottehead
Copy link

Hey, Im the latest requester for this. I scanned the fork and the PR. If I can help somehow let me know.

@jpodwys jpodwys merged commit 7680e43 into jpodwys:master Feb 5, 2020
@jpodwys
Copy link
Owner

jpodwys commented Feb 5, 2020

@valdemon @hottehead I've merged this and published it to NPM as 3.0.0-beta.1. I want to have a bit more time dive deep on a couple of things as well as updating some items in the readme before releasing as 3.0.0 but I didn't think that should stop people from trying it out!

I'd love to port these changes over to superagent-cache sometime soon as well. Not sure when I'll have time to work on that right now, though.

@valdemon thank you again for these changes! @hottehead thanks for the push to merge this in.

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.

3 participants