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
create vendor bundle with hand-picked libraries #30374
Conversation
}, | ||
vendors: { | ||
name: 'vendors', | ||
priority: 30, |
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.
What does this do?
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.
we discussed offline briefly. more details are here: https://webpack.js.org/plugins/split-chunks-plugin/#splitchunkscachegroupscachegrouppriority
This is cool! Can you share a before and after snapshot of the |
apps/Gruntfile.js
Outdated
_.keys(appsEntries), | ||
_.keys(pegasusEntries), | ||
_.keys(professionalDevelopmentEntries), | ||
_.keys(internalEntries) |
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: why not Object.keys
?
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
%script{src: minifiable_asset_path('js/code-studio-common.js')} | ||
%script{src: minifiable_asset_path('js/common.js')} | ||
%script{src: asset_path("js/#{js_locale}/common_locale.js")} | ||
%script{src: minifiable_asset_path('js/styleguide.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.
Removed because it already inherits from the application template?
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.
Removed because it unused and would require updating otherwise.
Caveat: I haven't tested this locally yet. Lmk if you'd like me to! |
I don't need you to test this locally unless you want to but thanks for the careful review! |
Background
One problem with our current webpack config is that shared modules are not pulled out of plc entry points. Because using count-based approaches is tricky, I am hand-picking the list of packages included here based on inspection of which packages are used by most entry points as well as which packages are aligned with our current best practices (e.g. react).
According to many sources on the internet, the strategy of bundling npm packages separately from application code should improve our caching performance because vendors.js is likely to remain cached (by Cloudfront, firewalls, browsers, etc.) for days or weeks, compared with our application code which updates daily.
Description
Extract a hand-picked list of npm packages from most dashboard and pegasus entry points. This does not include special entry points like applab-api.js and embedVideo.js which need to be entirely self-contained.
results
before
after
future work
there is some code still shared across many PLC entry points (e.g. react-bootstrap) which could benefit from being extracted into some kind of
plc-common.js
bundle. I didn't include this in vendors.js because react-bootstrap is not used widely outside of PLC code.Ideally, we'd be able to do something like this:
currently we use this strategy to avoid duplication between common.js and code-studio-common.js, however it is not clear whether the trick mentioned earlier will scale as the number of shared bundles increases.
I have a future work item to explore a more dynamic approach to bundle splitting which does not require our configuration to have specific knowledge of our application code, which is part of why I'm choosing this as a stopping point for now.