Improve caching policy use immutable when loading versionned assets #31141
Conversation
CarlSchwan
changed the title
CarlSchwan
changed the title
|
|
|
@acsfer I just read completely https://bugs.chromium.org/p/chromium/issues/detail?id=611416 and it seems to me like chromium has a heuristic to determine when it should try to see if the server updated the assets or not. According to the comments, it's not as good as the solution in firefox and safari but still ok. There were also some commenters (e.g. the BBC) who said that even if firefox was used by 10% of their users, it represented 30% of their 304 response code. So a nice improvement overall in my opinion for client responsivity and server load. |
|
@CarlSchwan no problem for |
PVince81
requested review from
ChristophWurst,
icewind1991,
juliusknorr and
Pytal
|
To fix? |
CarlSchwan
force-pushed
the
fix/better-cache-policy
branch
from
f221f1a
to
3922988
Compare
szaimen
added
the
pending documentation
|
Doc PR: nextcloud/documentation#8027 (I didn't test it, can an nginx expert try to see if the syntax is correct?) |
solracsf
removed
the
pending documentation
* Cache css with version in url. This makes most js and css requests to be cached by the browser * Force caching previews, the etag is in the url so that if the propfind gives a new etag, we will refresh it otherwise it's no use to try to fetch the new etag and do tons of DB queries Tested with firefox and 'debug' => false (important so that the js/css urls are generated with ?v= parameter) Signed-off-by: Carl Schwan <[email protected]>
CarlSchwan
force-pushed
the
fix/better-cache-policy
branch
from
3922988
to
7dddbd0
Compare
|
For the doc/nginx PR, a user told me it didn't work out for them. Could someone familiar with Nginx double-check? nextcloud/documentation#8027 |
|
New doc PR to fix the issue: nextcloud/documentation#8046 |
|
backport? |
|
/backport to stable23 |
|
/backport to stable22 |
| @@ -110,10 +110,10 @@ public function __construct() { | |||
| * @return $this | |||
| * @since 6.0.0 - return value was added in 7.0.0 | |||
| */ | |||
| public function cacheFor(int $cacheSeconds, bool $public = false) { | |||
| public function cacheFor(int $cacheSeconds, bool $public = false, bool $immutable = false) { | |||
There was a problem hiding this comment.
Shouldn't this be added to #29914 as it's changing public API (apps might extend Response to implement custom responses)? And backporting yields a new minor for the old stables?
There was a problem hiding this comment.
Bumping this due to the imminent release of the first NC24 beta and patch releases for old stables.
Breaking 3rd-party apps in patch releases is bad 😉
There was a problem hiding this comment.
Do you have an example of an app that did extend Response to modify the cacheFor method?
There was a problem hiding this comment.
Just to get this straight: I don't really have any issue with this change, it's fine, there must be a way to introduce features like this. However, it needs documentation and it must yield a new minor for the old stables, as app devs don't expect such things to change in patch releases.
There was a problem hiding this comment.
For others to follow: Also see nextcloud/cms_pico#195 for the ongoing discussion.
See nextcloud/server#31141 Signed-off-by: Daniel Rudolf <[email protected]>
See nextcloud/server#31141 Signed-off-by: Daniel Rudolf <[email protected]> (cherry picked from commit dd81fad)
Cache the CSS with the version in the url. This makes most js and CSS requests to
be cached by the browser
Force caching previews, the ETag is in the URL so that if the propfind
gives a new ETag, we will refresh it otherwise it's no use to try to
fetch the new ETag and do tons of DB queries
Tested with firefox and 'debug' => false (important so that the js/css
urls are generated with ?v= parameter)
Signed-off-by: Carl Schwan [email protected]