Client caching #1545
Client caching #1545
Conversation
This should make the vault pages load much faster, and massively reduce the number of requests.
The URLs are cachebusted, so updates will still be applied cleanly and immediately
src/api/icons.rs
Outdated
| @@ -51,7 +51,10 @@ fn icon(domain: String) -> Option<Cached<Content<Vec<u8>>>> { | |||
| return None; | |||
| } | |||
|
|
|||
| get_icon(&domain).map(|icon| Cached::long(Content(ContentType::new("image", "x-icon"), icon))) | |||
| get_icon(&domain).map(|(icon, cached)| { | |||
| let cache_ttl = if cached {CONFIG.icon_cache_ttl()} else {CONFIG.icon_cache_negttl()}; | |||
There was a problem hiding this comment.
Not sure this use of cached makes sense, if you return an icon, you should always use the icon_cache_ttl.
The case when icon_cache_negttl would be used is when the icon couldn't be found, so get_icon returns None, and Rocket returns a 404, no idea if the browsers respect the cache headers in the case of a 404 though.
There was a problem hiding this comment.
I think 404's arn't cached.
Also, the time should be calculated depending on the time of the icon file and the ttl set for this to work correctly.
There was a problem hiding this comment.
Ah yeah looks like I misread the implementation.
There was a problem hiding this comment.
@BlackDex I don't think it would be worth it to calculate the ttl based on the file date, at worst as it is now, a browser could cache a file for the full ttl while it's close to expired, which would mean it's outdated by an extra month by default if the clients respect the full ttl, but it would complicate the code for not much benefit, and I don't expect the favicons to change that much anyway.
Cache icons on the client too, to reduce requests when loading the homepage.
Caching without
immutablewill still issue requests, and because Rocket doesn't support conditional GETs, it's basically like not caching at all.This also caches vault static files, which are cachebusted anyway so updates will still work fine. I've not changed the
/route for this reason.Bitwarden upstream doesn't do these, but they also use cloudflare for a CDN which makes this far less of an issue. For home users who don't use a global CDN, this should make quite a difference!