Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serve static assets with an efficient cache policy #2216

Merged
merged 17 commits into from Mar 9, 2023
Merged

Serve static assets with an efficient cache policy #2216

merged 17 commits into from Mar 9, 2023

Conversation

mamico
Copy link
Member

@mamico mamico commented Feb 18, 2021

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

@tiberiuichim
Copy link
Contributor

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
Copy link
Contributor

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

@mamico
Copy link
Member Author

mamico commented Feb 24, 2021

@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...

@tiberiuichim
Copy link
Contributor

tiberiuichim commented Feb 24, 2021

@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?

@mamico
Copy link
Member Author

mamico commented Mar 16, 2021

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

@nzambello nzambello added this to In progress in RedTurtle priorities via automation Jun 22, 2021
@tisto
Copy link
Sponsor Member

tisto commented Jul 7, 2021

@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' }};
        }
        ...
    }

@mamico
Copy link
Member Author

mamico commented Jul 7, 2021

@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: #2216 (comment)

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.

@tisto
Copy link
Sponsor Member

tisto commented Jul 7, 2021

@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?

@mamico
Copy link
Member Author

mamico commented Jul 7, 2021

... 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.

@jensens
Copy link
Sponsor Member

jensens commented Jul 7, 2021

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.

@tiberiuichim
Copy link
Contributor

tiberiuichim commented Jul 7, 2021

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: #2545

@mamico
Copy link
Member Author

mamico commented Jul 8, 2021

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

@tiberiuichim
Copy link
Contributor

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.

@mamico
Copy link
Member Author

mamico commented Oct 12, 2021

@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

@tiberiuichim
Copy link
Contributor

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

@tiberiuichim tiberiuichim self-assigned this Nov 9, 2021
@mamico mamico requested a review from cekk April 27, 2022 14:55
@mamico
Copy link
Member Author

mamico commented Apr 29, 2022

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 #2804, I see an almost complete work done by @tiberiuichim on this. I could integrate into his branch after this is merged.

@netlify
Copy link

netlify bot commented Aug 29, 2022

Deploy Preview for volto canceled.

Name Link
🔨 Latest commit f52ec01
🔍 Latest deploy log https://app.netlify.com/sites/volto/deploys/640994212eeffe00070b4675

@cypress
Copy link

cypress bot commented Aug 29, 2022

Passing run #4306 ↗︎

0 485 20 0 Flakiness 0

Details:

Merge branch 'master' into maxage
Project: Volto Commit: f52ec013e8
Status: Passed Duration: 15:01 💡
Started: Mar 9, 2023 8:13 AM Ended: Mar 9, 2023 8:28 AM

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@wesleybl
Copy link
Member

wesleybl commented Sep 3, 2022

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.

@sneridagh sneridagh added this to the Plone Conference 2022 milestone Oct 1, 2022
@sneridagh sneridagh requested a review from ericof March 8, 2023 15:36
Copy link
Sponsor Member

@ericof ericof left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
(But we should document that somewhere)

@tiberiuichim
Copy link
Contributor

Apparently this is an issue for the EEA website as well. CC @avoinea

@sneridagh sneridagh merged commit 3ea8527 into master Mar 9, 2023
RedTurtle priorities automation moved this from Reviewer approved to Done Mar 9, 2023
@sneridagh sneridagh deleted the maxage branch March 9, 2023 09:54
sneridagh pushed a commit that referenced this pull request Mar 9, 2023
Co-authored-by: nzambello <nzambello@protonmail.com>
Co-authored-by: Tiberiu Ichim <tiberiu.ichim@gmail.com>
Co-authored-by: Tiberiu Ichim <tiberiuichim@users.noreply.github.com>
sneridagh added a commit that referenced this pull request Mar 9, 2023
Co-authored-by: Mauro Amico <mauro.amico@gmail.com>
Co-authored-by: nzambello <nzambello@protonmail.com>
Co-authored-by: Tiberiu Ichim <tiberiu.ichim@gmail.com>
Co-authored-by: Tiberiu Ichim <tiberiuichim@users.noreply.github.com>
@sneridagh
Copy link
Member

@mamico I merged and released, any chance that you could drop some documentation about the new setting? Thanks!

@stevepiercy
Copy link
Collaborator

@sneridagh @mamico at the least, please create an issue so that someone can pick it up, if no one can do it now.

sneridagh pushed a commit that referenced this pull request Mar 12, 2023
Co-authored-by: nzambello <nzambello@protonmail.com>
Co-authored-by: Tiberiu Ichim <tiberiu.ichim@gmail.com>
Co-authored-by: Tiberiu Ichim <tiberiuichim@users.noreply.github.com>
erral pushed a commit that referenced this pull request Mar 18, 2023
Co-authored-by: nzambello <nzambello@protonmail.com>
Co-authored-by: Tiberiu Ichim <tiberiu.ichim@gmail.com>
Co-authored-by: Tiberiu Ichim <tiberiuichim@users.noreply.github.com>
sneridagh added a commit that referenced this pull request May 10, 2023
* master: (23 commits)
  Fix training urls (Remove /5/) (#4502)
  Release 17.0.0-alpha.1
  Changelog
  Serve static assets with an efficient cache policy (#2216)
  Release 16.15.0. Update Changelog. (#4485)
  Improvements to dev proxy (#4434)
  fix: newsitem view blocks wrapper className (#4443)
  Fix history view dropdown for first entry, showing 'Revert to this version option' always (#4471)
  Teaser block docs (#4461)
  Fix order of row of long table  in edit and view mode. (#4472)
  Improve flaky test in autofocus Cypress tests (#4475)
  Use popperjs in BlockChooser, move the markup to the bottom of the bo… (#4141)
  Change from links to inline literals in `CHANGELOG.md` to fix linkche… (#4470)
  Release generate-volto 7.0.0-alpha.2
  Improve stylelint config in generator, include scss support (#4469)
  Fix weird GHA failure on config option not supported (#4466)
  Release generate-volto 7.0.0-alpha.1
  Fix ESlint failure for the generator (#4465)
  Change towncrier job name (#4462)
  Release 17.0.0-alpha.0
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants