-
Notifications
You must be signed in to change notification settings - Fork 480
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
better support of caching headers #28
Comments
Changes in 781938a (+shepik@ab12571#diff-0fecf8d8a1c3b52ef05e62d6a1bf6eff) could be considered for upstream. Basically, i hard-coded "always cache image from remote_url for 14 days" (so that 2 different resizes of that image will use 1 request to the remote server, regardless of its cache policy) |
was this a remote URL that wasn't setting its own cache headers? |
it was. |
hmm 😕 Not sure what I'd do in that case, since it hasn't actually been a problem for me personally yet. It's one thing if upstream specifically says "Cache-Control: no-cache", but if they don't have anything, then is it really a problem? And either way, you don't want to spin the CPU unnecessarily. However, it's unfortunate that it requires such deep changes into the httpcache library... I'll try and see if there's a less invasive way of achieving the same thing. |
Other Cache-Control headers to consider... how should imageproxy handle these?
(From https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Cache-Control#Other) |
@willnorris I would like to overwrite cache-control in case none is existent - through the config:
|
notably, we don't do anything directly with the
Cache-Control
. The httpcache library handles respecting upstream directives in terms of what gets cached, but we don't set any Cache-Control directives on downstream responses.@shepik, I see that you rewrote part of the httpcache library in 781938a, but I haven't yet looked closely at what you changed. Were these custom changes for something you're doing, or are these things that should be considered for inclusion upstream?
also /cc @jelinden who added Cache-Control support in e41beef for cloudfront. I'm not sure that we want to hard-code a max-age, as opposed to respecting the max-age from upstream, but I'd be curious to hear what cloudfront needs in this regard.
The text was updated successfully, but these errors were encountered: