Add Cache-control: immutable by rullzer · Pull Request #7904 · nextcloud/server

rullzer · 2018-01-16T21:49:07Z

If not in debug mode we basically cache the CSS for ever ;) Else we fall back to the default (1 day) in debug mode.

Most modern browsers support this. And on upgrade we have a cache buster. basically it is just telling supported browsers this will not change.

Todo:

JS endpoint

rullzer · 2018-01-17T14:47:25Z

@MorrisJobke @nickvergessen @ChristophWurst @skjnldsv how do you all feel about this? It would basically omit refetching of the files if the browser supports it (most new ones do) if the hash doesn't change.

Personally I don't really care that much as we cache them for a day now already. And re validating is done based on Etag. This would just be another optimization in that direction.

skjnldsv · 2018-01-18T11:39:29Z

I'm in, seems like the proper way to do it. But yes, we override the cache by switching the hash in updates so we should be clear here :)

rullzer · 2018-01-18T15:31:39Z

Ok then let me pick this up later today so we can get it in :)

rullzer · 2018-01-18T21:17:04Z

So second question. do we want different behaviour in debug mode? Currently we did not and it was still cached for a day. Which seems long. We could also switch to this by default. Less code paths is less things that can do 💥.

MorrisJobke · 2018-01-18T21:18:39Z

@skjnldsv Could you give me an overview of how changes are detected? because I doubt that we are looking for the mtime for all involved files on every request, right?

rullzer · 2018-01-18T21:29:26Z

@MorrisJobke the hash is generated from the appversion it is part of.

We could check the mtime (the SCSSCacher has all this info). But of course that would kind of kill performance.

Maybe it is best to have guidelines

During development people will mostlikely force refresh anyways
Changes in css or js then require version bumps to propagate properly

Note that this PR just changes the duration. Not the way things work. Right now people can also have outdated files on their dev instances for a day if they don't force refresh ;)

rullzer · 2018-01-18T21:29:58Z

Ah better idea. We could just generate a random hash when debug mode is set? Sure it sucks speed wise. But will work all the time.

rullzer · 2018-01-26T15:18:49Z

Ok let me convert this jsut to always use immutable. And then we tackle the other problems some other way.

My reasoning:

We already cache for a day
If you are having issues you will have refreshed properly already
Better to just tell the browser to cache like crazy ;)

codecov · 2018-03-08T08:50:31Z

Codecov Report

❗ No coverage uploaded for pull request base (master@734cddf). Click here to learn what that means.
The diff coverage is 78.57%.

@@ Coverage Diff @@## master #7904 +/- ##
=========================================
Coverage ? 51.86% Complexity ? 25377 =========================================
Files ? 1608 Lines ? 95178 Branches ? 1377 =========================================
Hits ? 49366 Misses ? 45812 Partials ? 0

Impacted Files	Coverage Δ	Complexity Δ
core/Controller/JsController.php	`93.54% <100%> (ø)`	`7 <6> (?)`
core/Controller/CssController.php	`85.29% <66.66%> (ø)`	`7 <7> (?)`

rullzer · 2018-03-08T08:54:50Z

We have #7244

So just tell the browser to do aggressive caching. In debug mode you don't request those files anyway and just fetch the plain files.

Ready for review!

ChristophWurst · 2018-03-08T09:06:40Z

 }
- $response->cacheFor(86400);
+
+ $ttl = 365000000;


Magic number? 😉

Not really ... just roughly 41667 years 😜

Maybe set it to a year: 8760 and until then the instance is updated anyways. ;)

I was wrong - as this is seconds it is only a bit over 11,5 years

It was is used in all the example of the immutable object

Could we lower it to 1 year?

Sure. I mean it doesn't really make a difference. But as you wish 😉

ChristophWurst · 2018-03-08T09:07:50Z

 }
- $response->cacheFor(86400);
+
+ $ttl = 365000000;


MorrisJobke

Tested and works 👍 I tested with and without debug mode and both have the same behavior. Beside that the cache now needs to be cleared (or the apps version should be updated) if you want to test new (S)CSS/JS for example.

rullzer · 2018-03-08T12:30:44Z

@MorrisJobke well before we also said (without debug mode) cache this for 24 hours. But I guess if you are actively developping you should hard refresh anyways. As I have seen browsers already honour our 24h caching.

MorrisJobke · 2018-03-08T12:47:00Z

@MorrisJobke well before we also said (without debug mode) cache this for 24 hours. But I guess if you are actively developping you should hard refresh anyways. As I have seen browsers already honour our 24h caching.

Sure :) Was just a clarification comment.

Cache generated CSS forever! Also cache combined JS forever Fix tests Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

ChristophWurst

Fine by me

rullzer added enhancement 2. developing Work in progress labels Jan 16, 2018

rullzer added this to the Nextcloud 14 milestone Jan 16, 2018

rullzer force-pushed the fix_7224 branch from 5bd60e9 to d7d6590Compare March 8, 2018 08:50

rullzer force-pushed the fix_7224 branch from d7d6590 to b262b6eCompare March 8, 2018 08:53

rullzer requested review from ChristophWurst, MorrisJobke, juliusknorr and skjnldsv March 8, 2018 08:55

rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 8, 2018

ChristophWurst reviewed Mar 8, 2018
View reviewed changes

MorrisJobke approved these changes Mar 8, 2018
View reviewed changes

rullzer force-pushed the fix_7224 branch from b262b6e to afb1118Compare March 8, 2018 20:22

Add Cache-control: immutable
796b4f1
Cache generated CSS forever! Also cache combined JS forever Fix tests Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer force-pushed the fix_7224 branch from afb1118 to 796b4f1Compare March 19, 2018 13:22

ChristophWurst approved these changes Mar 19, 2018
View reviewed changes

MorrisJobke merged commit c8340ac into masterMar 21, 2018

MorrisJobke deleted the fix_7224 branch March 21, 2018 07:27

Uh oh!

Uh oh!

Conversation

rullzer commented Jan 16, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rullzer commented Jan 17, 2018

Uh oh!

skjnldsv commented Jan 18, 2018

Uh oh!

rullzer commented Jan 18, 2018

Uh oh!

rullzer commented Jan 18, 2018

Uh oh!

MorrisJobke commented Jan 18, 2018

Uh oh!

rullzer commented Jan 18, 2018

Uh oh!

rullzer commented Jan 18, 2018

Uh oh!

rullzer commented Jan 26, 2018

Uh oh!

codecovBot commented Mar 8, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

rullzer commented Mar 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MorrisJobke left a comment

Choose a reason for hiding this comment

Uh oh!

rullzer commented Mar 8, 2018

Uh oh!

MorrisJobke commented Mar 8, 2018

Uh oh!

ChristophWurst left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

rullzer commented Jan 16, 2018•
edited
Loading

codecovBot commented Mar 8, 2018•
edited
Loading