-
Notifications
You must be signed in to change notification settings - Fork 143
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
11670 Make webpack add filehashes to all (read: entrypoint) js files in build #11724
base: dev/8.0.x
Are you sure you want to change the base?
Conversation
This reverts commit 45947d2. revert
…nto archesproject-dev/8.0.x pulls latest from head
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks pretty good. I like the concept and I get why this needs to be a things. There are a few pattern issues to address, but I'm glad you've got this ball rolling! 🍙
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is being added to both webpack.config.dev.js
and webpack.config.prod.js
, it instead belongs in webpack.common.js
@@ -88,7 +88,8 @@ | |||
"uuidjs": "^3.3.0", | |||
"vue": "^3.5.8", | |||
"vue3-gettext": "^3.0.0-beta.6", | |||
"webpack-bundle-tracker": "^3.1.0" | |||
"webpack-bundle-tracker": "^3.1.0", | |||
"webpack-manifest-plugin": "5.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should instead be a dev dependency
@@ -41,6 +42,18 @@ module.exports = () => { | |||
PROJECT_RELATIVE_NODE_MODULES_PATH = Path.resolve(APP_ROOT, '..', 'node_modules'); | |||
} | |||
|
|||
const manifestPlugin = new WebpackManifestPlugin({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: inline in plugins
@@ -41,6 +42,18 @@ module.exports = () => { | |||
PROJECT_RELATIVE_NODE_MODULES_PATH = Path.resolve(APP_ROOT, '..', 'node_modules'); | |||
} | |||
|
|||
const manifestPlugin = new WebpackManifestPlugin({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There appears to be a large overlap between what this generates and what's already created in webpack/webpack-stats.json
. Is there a chance to combine them?
@@ -109,3 +113,25 @@ def app_settings(request=None): | |||
"DEBUG": settings.DEBUG, | |||
} | |||
} | |||
|
|||
|
|||
def webpack_manifest(request): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why make this a context_processor?
main_script = kwargs.get("main_script", None) | ||
if main_script: | ||
context["main_script_webpack_asset"] = context["webpack_manifest"].get( | ||
f"{main_script}.js", "" | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doesn't this need error handling as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hard part here is that there's no guarantee and endpoint will use BaseManager
anymore.
Types of changes
Description of Change
This PR creates a webpack_manifest (essentially just a lookup for files to their actual webpack asset filename) so that all
.js
files can be saved using a filehash in the filename, thereby forcing a cache burst on end-user browser clients whenever novel changes get pushed to a deployment. This is effective on both dev and prod builds.The
base.py
view now looks up the asset corresponding to themain_script
value.See webpack-manifest-plugin for more docs
Note that there is an assumption in where the manifest should be looked for:
...and it's possible this should search a list of locations instead.
Issues Solved
Closes #11670
Testing this PR
You will need to add:
to your project dependencies and run
npm install
to install webpack_manifest. You'll also want to run both a production and development build of webpack. All "main_views" should be tested, e.g.Checklist
Accessibility Checklist
Developer Guide
Ticket Background
Further comments