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

ManifestStaticFilesStorage cache-busting doesn't work for JavaScript imports #1808

Closed
richardebeling opened this issue Sep 12, 2022 · 4 comments · Fixed by #1954
Closed

ManifestStaticFilesStorage cache-busting doesn't work for JavaScript imports #1808

richardebeling opened this issue Sep 12, 2022 · 4 comments · Fixed by #1954
Assignees
Labels
[C] Infrastructure Infrastructural work around the main EvaP project [P] Medium Medium priority [T] Bug This is a bug. We don't like it. Please get rid of it.
Milestone

Comments

@richardebeling
Copy link
Member

richardebeling commented Sep 12, 2022

We currently use the ManifestStaticFilesStorage to have long browser cache times and cache-busting after updates. On collectstatic, it creates a copy of all files that also contains the file hash in the name -> /static/css/evap.css becomes /static/css/evap.bfc8f42bfc39.css". In django templates, we use the static template tag to mark places where this replacement is necessary.
In CSS files, relative file names are automatically changed.

However, this doesn't work for javascript files that import each other. For example, our current contact_modal.js imports stuff from utils.js. This import statement is missing the hash in the file name.
The access will work, because the files are also available under their original name, but browsers might use the old, cached files.

Currently, production serves files with a one year cache expiring time (3 hours for the raw files without hashes in the name).
So, after an update, users might have broken javascript until that time runs out or they refresh with Ctrl+F5.

Options:

  • Reduce the cache time, make the browser re-check more often (~10minutes?). Ensure that a re-check is a simple "nothing has changed" from the webserver (and is fast). Our webserver already sends out ETags with the requests. This would allow us to drop the ManifestStaticFilesStorage (or reduce it to only css and image files). I prefer this option.

  • We considered whether we can have a "dear browser, the website was last updated on X, please discard all earlier cached files" header, so that browsers recheck files where they have older versions, but it doesn't seem like there is such a header available in HTTP.

  • We could also manually change import-statements in js/ts files, maybe by changing the ManifestStaticFilesStorage, but it would probably be painful and error-prone.

    • We think we can not mark these places, like with the static template tag, because it would break javascript tooling.
  • JS tooling could probably solve this (webpack, ...). This probably exists somewhere on the internet, collectstatic could then run this. However, in my experience, javascript packaging always is a pain, so I'd vote to not do that.

@niklasmohrin @karyon @janno42 opinions?

@richardebeling richardebeling added [T] Bug This is a bug. We don't like it. Please get rid of it. Discussion This issue requires some discussion and a decision what a solution might look like. [C] Infrastructure Infrastructural work around the main EvaP project labels Sep 12, 2022
@richardebeling richardebeling changed the title ManifestStaticFilesStorage and JavaScript imports ManifestStaticFilesStorage cache-busting doesn't work for JavaScript imports Sep 12, 2022
@karyon
Copy link
Collaborator

karyon commented Sep 12, 2022

As far as I can tell, production serves files with a 1 year max-age. That was the point of having hashes in the filenames: to enable long cache expiration dates so the browser doesn't need to send a request for that file at all.
image

Regarding your first option: That would certainly work and be very simple and probably barely noticeable by the user, but I could imagine it introduces additional roundtrips to the server before rendering the page starts.

Regarding the second option, I'm not sure how a last-modified header would help but there is one and production also responds with it: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Last-Modified

Regarding your third option: The ManifestStaticFilesStorage does fix import paths for source maps and css imports already, see the ManifestStaticFilesStorage docs. Fixing paths of ES module imports is a missing feature, see https://code.djangoproject.com/ticket/32319. There was an attempt to fix this in this django PR. Example code how this additional regex might be used is here: https://code.djangoproject.com/ticket/32383. This was not merged because of unhandled edge cases, but it might be enough for us.

Personally I would prefer trying out the third option, if there's a way to prevent access to the original file name so we can catch this error earlier if this breaks down again.

@richardebeling
Copy link
Member Author

As far as I can tell, production serves files with a 1 year max-age. That was the point of having hashes in the filenames: to enable long cache expiration dates so the browser doesn't need to send a request for that file at all.

Ah, I probably checked the wrong file: Yes, all files with hashes in the name are currently served with an expiration of one year. The original files without a hash in the name are served with an expiration of 3 hours.
https://github.com/e-valuation/EvaP/pull/1529/files#diff-4d64646f3ddb433e7cabd98f95d12d2886a06b1f461ff2e1b66fdfb1045aa374R11

code.djangoproject.com seems to be down currently, will have to look into that later. If the code works for us and we have known edge cases where we know it will break, I'd also be fine with that.

Updated the text above for option 2 -- "last-modified" was a wrong description sine that exists already. The idea was to have a "dear browser, please discard all cached entries older than timestamp" header. But I don't know of any such header.

Regarding the delayed page load on option 1: From within the HPI network, an "If-Modified-Since" request to revalidate whether the current cached version is still up to date takes 5 to 8 milliseconds. Round trip times may be higher from external networks though. This would then be added to one page load every ten minutes, increasing the time-to-interactive for that page load.

@janno42 janno42 added the [P] Medium Medium priority label Sep 26, 2022
@janno42 janno42 added this to the Winter 2022 milestone Sep 26, 2022
@janno42 janno42 modified the milestones: Winter 2022, Summer 2023 Feb 4, 2023
@richardebeling
Copy link
Member Author

The django issue linked above was closed as fixed 3 months ago, and the preliminary release notes for django 4.2 contain this:

ManifestStaticFilesStorage now replaces paths to JavaScript modules in import and export statements with their hashed counterparts.

So this might just resolve with django 4.2

@richardebeling
Copy link
Member Author

Updated release notes for Django 4.2:

ManifestStaticFilesStorage now has experimental support for replacing paths to JavaScript modules in import and export statements with their hashed counterparts. If you want to try it, subclass ManifestStaticFilesStorage and set the support_js_module_import_aggregation attribute to True.

@janno42 janno42 removed the Discussion This issue requires some discussion and a decision what a solution might look like. label May 15, 2023
@richardebeling richardebeling self-assigned this May 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[C] Infrastructure Infrastructural work around the main EvaP project [P] Medium Medium priority [T] Bug This is a bug. We don't like it. Please get rid of it.
Development

Successfully merging a pull request may close this issue.

3 participants