search

getsentry / sentry

Developer-first error tracking and performance monitoring
https://sentry.io
Other
39.05k stars 4.19k forks source link

enable Cache-Control immutable by default for versioned assets (and on getsentry.com) #4842

Open graingert opened 7 years ago

graingert commented 7 years ago

Prevent revalidations to hash-busted or versioned static assets with Cache-Control immutable:

https://bitsup.blogspot.co.uk/2016/05/cache-control-immutable.html

mattrobenolt commented 7 years ago

Why is what we currently have not enough? On sentry.io, my browser's cache is just used correctly and doesn't issue a 304.

graingert commented 7 years ago

It does for me, try a refresh with the network panel open.

also would be good to have on https://cdn.ravenjs.com/3.9.1/raven.min.js

mattrobenolt commented 7 years ago

I think the behavior of "refresh" explicitly triggers assets to be attempted to be refetched. But normal browsing behavior doesn't cause that.

graingert commented 7 years ago

I would often refresh to see new sentry.io events/errors in my stream, also https://cdn.ravenjs.com/3.9.1/raven.min.js might be used on other sites where users refresh streams.

mattrobenolt commented 7 years ago

Also, fwiw, on Chrome refreshing the page doesn't trigger a revalidation request. I can't actually trigger a revalidation or refresh unless I explicitly clear cache or disable cache.

So this seems something wrong in why your cache isn't behaving correctly or respecting the current Cache-Control rules that say to use the cache.

graingert commented 7 years ago

I think Chrome is breaking the rules here, as it's supposed to revalidate. Immutable is just there to bring this behaviour to other browsers.

mattrobenolt commented 7 years ago

I'm going to look into this because I find this hard to believe, considering I've spent a good amount of effort around this stuff in the past.

Saying max-age=x is explicitly telling the browser that it doesn't need to revalidate or check for an update in x seconds. This immutable tag just seems to be an explicit definition for this since it's common practice. But I don't see how what we have currently is incorrect or wouldn't be behaving as I'm expecting.

BYK commented 4 years ago

FYI Chrome is not willing to support this at all: https://bugs.chromium.org/p/chromium/issues/detail?id=611416

We may want to do this for other browsers tho. WDYT @mattrobenolt @JTCunning?