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

New javascript/css files are not loaded because index.html is cached #2483

Closed
runephilosof-karnovgroup opened this issue Sep 20, 2023 · 8 comments · Fixed by #2975
Closed
Labels
enhancement improve existing features ui frontend related

Comments

@runephilosof-karnovgroup
Copy link
Contributor

indexHTML should not be cached with a long expiry.
index.html references which version of the javascript and css files should be loaded.
So when index.html is cached with a long max-age the browser will not notice when it is time to load new javascript/css.

c.Writer.Header().Set("Cache-Control", "public, max-age=31536000")

Users need to reload the page to get the new js/css files. And even worse, if they do that while on /user for instance, the browser will only cache the new js/css files when you load woodpecker on /user next time. If you go to /repos or /, your browser will still have cached the old index file for those endpoints...

@runephilosof-karnovgroup
Copy link
Contributor Author

Recently experienced problems with outdated javascript code again in #2821 (comment)

@qwerty287
Copy link
Contributor

Is there a way to use the etag we add as header here? (Md5 of server startup time). I.e. always reload the index if this has changed?

@qwerty287 qwerty287 added ui frontend related enhancement improve existing features labels Nov 21, 2023
@runephilosof-karnovgroup
Copy link
Contributor Author

If Cache-Control is set to max-age=0, then etag will be used.
But this is only part of the problem.
Since a lot of the pages changes are within the spa, then the index.html won't get loaded, just new json responses.
So the server should probably set a header for the json responses with its version, to let the frontend detect whether it is outdated.
Or the spa should poll /version every minute or so.

@qwerty287
Copy link
Contributor

qwerty287 commented Nov 22, 2023

If index.html is reloaded, also every spa file that has been changed will be reloaded, because they have a hash in their filename that changes in this case.

@runephilosof-karnovgroup
Copy link
Contributor Author

But index.html is not reloaded, because max-age=31536000.
So the end user won't get the new version unless they manually click reload in the browser (it just happened to me again, when upgrading to v2.0.0).

It is fairly easy to reproduce.

  1. Install next on the server.
  2. Visit in a browser.
  3. Change server to v2.0.0.
  4. Click around in the browser
  5. Open a new window, go to your woodpecker installation

Actual:
Version still shows next in the top left

Expected:
Version shows v2.0.0 in the top left

@qwerty287
Copy link
Contributor

Would this issue be solved by just removing the max-age and only set etag?

@runephilosof-karnovgroup
Copy link
Contributor Author

max-age=0, or a low number.
This will affect how many requests the server receives for assets (those requests will have If-None-Match: <etag>, so probably easily processed, but they will still have to be processed).
Assets which have hashes, timestamps or similar in the request, can be cached a long time.

Some assets don't have cache busting hashes/timestamps, like the favicon. So setting max-age=0 will make it easier to update those as well. Although, adding cache busting to those assets will be even better.

runephilosof-karnovgroup added a commit to runephilosof-karnovgroup/woodpecker that referenced this issue Dec 19, 2023
Or at least only cache it a short while. Set caching to 1 minute.
Should not increase server load much, since most page changes are with the javascript spa app and does not ask for index.html.

This will ensure that the right version of the javascript file is loaded.

Fixes woodpecker-ci#2483
@runephilosof-karnovgroup
Copy link
Contributor Author

It seems the etag is only set. It is never checked by the server when the browser sends
If-None-Match: <etag>.
Anyway, I have made a PR for setting the max-age: 60 when it is the index.html.

lafriks added a commit that referenced this issue Dec 20, 2023
Replaces #2972

Fixes #2483

Removed etag header as etag is used incorrectly, it should be based on
content not startup time and we don't handle it from request headers
anyway.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement improve existing features ui frontend related
Projects
None yet
2 participants