October: [feature-request] make `combine` cacheable

[zerkms]

At the moment the /combine/xxx requests served with combined assets (js or css) contain the

Cache-Control:private, must-revalidate

header.

Which means every client requests the script every time. That makes the combiner do the whole minification + combination + everything else job every request.

It still saves few traffic for the clients, indeed.

But what if instead the request was cached forever (or for a configurable time interval)? That way the sites became more responsive and we would save some CPU cycles + server IO.

Thoughts?

[daftspunk]

Have you tried enabling this setting?

/*
|--------------------------------------------------------------------------
| Determines if the asset caching is enabled.
|--------------------------------------------------------------------------
|
| If the caching is enabled, combined assets are cached. If a asset file
| is changed on the disk, the old file contents could be still saved in the cache.
| To update the cache the back-end Clear Cache feature should be used. It is recommended
| to disable the caching during the development, and enable it in the production mode.
|
*/

'enableAssetCache' => false,

[zerkms]

I have not and for some reason I did not notice it. Will check in monday and close this issue then, thanks

[zerkms]

Well, the provided answer just partially addresses my feature request:

1. The enableAssetCache has no influence on serving combined assets. The only place where it is used is for generating the combined asset urls (in the CombineAssets::prepareRequest)
2. It still should go through the Controller::combine (that includes reading the minified contents from cache and joining them in memory), while we can cache the entire response forever on the client side (that's what I have actually done with nginx)

[daftspunk]

Not sure what you mean, thanks to the etag setting here. I am getting "304 Not Modified" header on asset files that have not been changed. You are right though, the cache setting has no impact on this feature.

Just one thing to check, make sure you don't have the Disable cache checkbox ticked in Chrome tools (if this is what you are using) as it will suppress the not modified behavior.

Otherwise, what are you proposing that we change?

[zerkms]

Well, take a debugger and go through what code is evaluated on every request.

Indeed, it is 304 returned but on the server side there is still a lot of work performed, which might have been avoided (if resources were cached forever unconditionally).

[daftspunk]

Great catch! This fix will be available in Build 383+

[jrbecart]

The fix doesn't work for me I still have the tag: Cache-Control:private, must-revalidate
for combined css and js.

All other tags are fine (ETag and Last-modified) but browser doesn't cache combined css and js.

My solution is to add:

 $response->header('Cache-Control', 'public');

in function getContents from CombineAssets.php

Is it a (good) solution ?

[LukeTowers]

@jrbecart I'll test it out locally and see if that fits. Thanks for pointing it out!

[daftspunk]

Interesting, this seems to work locally but on a remote (public) website, the 304 Not Modified headers are not being applied and a 200 code is sent.

[daftspunk]

If at first you don't succeed, try try try again (8aa9d25a913e471723527e0b88340dc3fc47821d).

[alekseyp]

@LukeTowers check my comment plz -
https://github.com/octobercms/october/commit/13596c8629e522e20aa21635c802405f2d1dfa98#r32340372