volto icon indicating copy to clipboard operation
volto copied to clipboard

Serve static assets with an efficient cache policy

Open mamico opened this issue 3 years ago • 19 comments

Added directive to cache stable resources in the browser or an intermediate server for 365 days, the static resource that could change after a new deployment for 1 minute

mamico avatar Feb 18 '21 08:02 mamico

My personal wish (and regret that I haven't done that when I was doing the refactoring) is that the static file handler would be moved to its own separate middleware in the express middleware folder.

tiberiuichim avatar Feb 19 '21 12:02 tiberiuichim

@mamico What is the scenario that you need the caching policy set in Volto? Usually we do this at the proxy http server level.

tiberiuichim avatar Feb 23 '21 17:02 tiberiuichim

@mamico What is the scenario that you need the caching policy set in Volto? Usually we do this at the proxy http server level.

@tiberiuichim, I agree with you: that is possible to set/override headers at (one of) the proxy HTTP level(s). But I think that could be better to have the correct cache headers set by the application. The same path was followed by p.a.caching, former all the works were done by complex, and sometimes unmanageable, varnish or squid configuration, now that the caching headers are managed by p.a.caching it's easier to configure varnish, Nginx or Cloudflare or whatever. That's my opinion. Instead, no problem to move the static handler to the middleware folder, if you want I could do that next week.

p.s. in this specific scenario where I'm working now, the HTTP reverse proxy is not managed by us, but by the customer. But it's not a usual use case...

mamico avatar Feb 24 '21 09:02 mamico

@mamico So, how about moving the static handler to a separate middleware inside express-middleware and make the caching configurable? I can imagine the following configuration:

config.settings.staticfiles = [{
    match: /.*/,
    headers: {
        'Cache-Control': 'public, max-age=31536000'
    }
}]

@nzambello @sneridagh @avoinea what do you think?

tiberiuichim avatar Feb 24 '21 09:02 tiberiuichim

@tiberiuichim moved to express middleware and configurable. Feel free to change anything.

mamico avatar Mar 16 '21 12:03 mamico

@mamico can you elaborate on why you prefer to cache static resources in Volto itself instead of in the webserver (e.g. Nginx)? We usually cache statice resources 1y in our Nginx config:

    location ~ / {
        location ~* \.(js|jsx|css|less|swf|eot|ttf|otf|woff|woff2)$ {
            add_header Cache-Control "public";
            expires +1y;
            proxy_pass http://{{ service_name + '-volto' }};
        }
        location ~* static.*\.(ico|jpg|jpeg|png|gif|svg)$ {
            add_header Cache-Control "public";
            expires +1y;
            proxy_pass http://{{ service_name + '-volto' }};
        }
        ...
    }

tisto avatar Jul 07 '21 06:07 tisto

@mamico can you elaborate on why you prefer to cache static resources in Volto itself instead of in the webserver (e.g. Nginx)? We usually cache statice resources 1y in our Nginx config:

@tisto I had already done it here: https://github.com/plone/volto/pull/2216#issuecomment-784923502

My opinion in short is that the "batteries included" approach would be better, otherwise we need to document / help for different proxy configurations (nginx, apache, varnish, haproxy, ...). Furthermore, following the suggestion of @tiberiuichim, the implementation is configurable and eventually with config.settings.staticfiles = [] you have the previous behavior.

mamico avatar Jul 07 '21 08:07 mamico

@mamico IMHO there is no way around a proper frontend configuration when you run Volto in production. We need to indeed document how to do this (this is on my todo list for quite a while now). Though adding this option in the Volto middleware might confuse people and send the wrong message that this is the recommended way to do things. Maybe I am missing something here though...

@sneridagh @jensens opinions?

tisto avatar Jul 07 '21 08:07 tisto

... Though adding this option in the Volto middleware might confuse people and send the wrong message that this is the recommended way to do things.

@tisto So, having this implementation with config.settings.staticfiles = [] as default (no effective changes with current implementation) could be a good way for you? People will be free to choice to implement caching rules in the proxy or in volto.

mamico avatar Jul 07 '21 09:07 mamico

So, I do not know much about Volto static resources. I few thoughts:

  • before caching them for a long time in browser, we must assure that after updates of Volto they are served under a new path (like timestamped directories or such for each new build/change)
  • If this is a static files directory, why then matching extensions? In fact everything in there has to be static.
  • globally matching extensions as in the Nginx example above is probably not that clever and is a gateway emitting side effects.
  • regarding where to set the headers: I think having it as a middleware has some advantages, but it has to be a no-brainer to switch it off.

jensens avatar Jul 07 '21 12:07 jensens

The bottom line is this, though: we have an HTTP server in Volto and it serves, by default, static resources. Even more, moving it to a separate middleware is needed for consistentency (and then, in keeping with Plone's example, most things should be configurable)

So I think the least we can do is move the static file to a separate middleware, then it can be overridden with one that also does caching headers, if such thing is needed.

Edit: I forgot to add this related issue: https://github.com/plone/volto/issues/2545

tiberiuichim avatar Jul 07 '21 18:07 tiberiuichim

So I think the least we can do is move the static file to a separate middleware, ...

@tiberiuichim Is this the implementation you are thinking about? https://github.com/plone/volto/pull/2216/files#diff-e060d0e91dab49476d9e335f020baafd48239c4143f80f4d1e47f75310dc44ffR1

mamico avatar Jul 08 '21 00:07 mamico

So I think the least we can do is move the static file to a separate middleware, ...

@tiberiuichim Is this the implementation you are thinking about? https://github.com/plone/volto/pull/2216/files#diff-e060d0e91dab49476d9e335f020baafd48239c4143f80f4d1e47f75310dc44ffR1

Yes, indeed, that's it. By having it moved into the config.settings.expressMiddleware it becomes possible to manipulate that list and create a new static files middleware that takes over.

tiberiuichim avatar Jul 08 '21 04:07 tiberiuichim

@tiberiuichim @tisto is there any news on this? I saw a conflict on server.js, if you think the PR is still valid I fix it. Thx

mamico avatar Oct 12 '21 12:10 mamico

I'm +1 on having the PR updated and merged. @plone/volto-team will probably agree?

tiberiuichim avatar Oct 12 '21 13:10 tiberiuichim

Is there any news on this? I saw a conflict on server.js, if you think the PR is still valid I fix it. Thx.

@sneridagh no problem to reworking the middleware after the implementation of https://github.com/plone/volto/pull/2804, I see an almost complete work done by @tiberiuichim on this. I could integrate into his branch after this is merged.

mamico avatar Apr 29 '22 07:04 mamico

Deploy Preview for volto canceled.

Name Link
Latest commit a753615b9b044cc5f1e17cdaa475156a9d0a4c75
Latest deploy log https://app.netlify.com/sites/volto/deploys/630d3aa25d27bf00090a5deb

netlify[bot] avatar Aug 29 '22 22:08 netlify[bot]



Test summary

347 0 15 0


Run details

Project Volto
Status Passed
Commit a753615b9b
Started Aug 29, 2022 10:19 PM
Ended Aug 29, 2022 10:30 PM
Duration 11:03 💡
OS Linux Ubuntu - 20.04
Browser Chrome 104

View run in Cypress Dashboard ➡️


This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. You can manage this integration in this project's settings in the Cypress Dashboard

cypress[bot] avatar Aug 29 '22 22:08 cypress[bot]

Furthermore, following the suggestion of @tiberiuichim, the implementation is configurable and eventually with config.settings.staticfiles = [] you have the previous behavior.

@mamico It would be nice to document this somewhere.

wesleybl avatar Sep 03 '22 14:09 wesleybl