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

Add hooks for caching responses #142

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

wakecoder
Copy link

This fork adds hooks (readCache and writeCache) that allow the user to cache responses and serve cached responses if they exist. It is up to the user to implement the caching system, this simply provides the hooks.

I'm using this in a project to cache WMTS requests and it works great.

@monkpow
Copy link
Collaborator

monkpow commented Nov 2, 2016

@wakecoder Thanks for the patch. I'm reviewing it.

@monkpow
Copy link
Collaborator

monkpow commented Nov 2, 2016

@wakecoder
Ok, this is nice looking change with tests, so I really appreciate that.

I have some questions about the utility of this. My goal is to keep this lib as simple and focused as possible, and I don't immediately see why the proxy lib should handle caching.

IOW. couldn't the same goal be accomplished via:

app.use(maybeUseCachedResponse(req, res, next) {
  // pseudo
  if (cacheHit) { res.send(cacheHit) }
  else { next() }
})

app.use(proxy(...))

There is an existing hook called intercept that allows you to operate on the proxiedResponse before its sent; it seems like this hook could be used to write to whatever caching lib one is using.

My concern is that caching begats cache-busting and a number of concerns that are not strictly related to proxying.

Happy to discuss further, and glad this fork works well for your use case, but at the moment, I'm not convinced this matches the goals of the library.

Thanks!

@wakecoder
Copy link
Author

@monkpow
Thanks for taking the time to look at it. This is a great library - easy to use and well-written.

Proxying and caching seem like fairly complementary ideas to me (see Squid for example) so it doesn't feel like a stretch to add hooks for it.

The approach you suggested would work but I wouldn't be able to make use of some of the internal functionality that express-http-proxy provides (e.g. copying headers, etc.). Admittedly, this isn't a significant amount of code.

Also, express-http-proxy unzips the response before calling the intercept function which may not make sense for caching. This fork will call the writeCache function without modifying/unzipping the response.

Lastly, without these hooks, if you want to cache, you have to register functions with two different libraries (express and express-http-proxy) which introduces implicit coupling between the two middlewares if I want to cache..

Thanks again for taking the time to look at it. Please let me know what you decide.

@monkpow
Copy link
Collaborator

monkpow commented Nov 9, 2016

@wakecoder Thanks for your thoughtful comments. What you're saying makes sense to me, but I'm still not convinced it fits; however, let me spend some time with this, and see if I can get it to fit a little better. I'm thinking about a more general wrapping hook that would allow you to insert or extract a response, that could be used for caching or additional purpose.

@wakecoder
Copy link
Author

@monkpow Sounds good. Let me know if I can help out somehow.

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.

2 participants