-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Fixes excluding js files from bundles when minifying is enabled #24506
Fixes excluding js files from bundles when minifying is enabled #24506
Conversation
Hi @hostep. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
Hi @VladimirZaets, thank you for the review.
|
@VladimirZaets: before this gets merged, I'd like some feedback on the removal of jquery from the bundle, I think that was incorrect and it should be reverted, what do you think? |
@hostep thanks for working on this. You'll definitely want |
f5aa1cd
to
cbe25f4
Compare
Thanks @DrewML for feedback, I agree with you. Just pushed an update to no longer remove jquery from the bundle. Only the minified file is now excluded, but since only one of both should be in the bundle, this makes sense. So nothing changed in that regards with how it was before. Also updated the table above with results of this change. |
✔️ QA Passed |
Hi @hostep, thank you for your contribution! |
Description (*)
When enabling both JS minification and JS bundling, there was as bug with determining files to be excluded from the bundled files.
From what I understand, it works like this when executing static content deployment:
pub/static/
per theme and locale.min.js
filenamepub/static/
and then match those filenames against the files of files in theview.xml
to determine if they should be included or excluded from the bundleThe problem here, is the fact that the
view.xml
file contains the original filenames, but the JS minification renames the files from.js
to.min.js
so it was no longer finding the correct files to exclude.(I'm almost convinced this used to work perfectly fine in Magento 2.1.x, because I remember me looking at this a couple of years ago and noticing no problems with both bundling and minification enabled, but I'm not 100% sure)
This problem is fixed in the first commit of this PR.
The fix is actually the one suggested by @SKovbel in #4506 (comment)
The second commit adds jquery to be excluded, since it exists in two variants in the codebase, which is super weird.
@DanielRuf also discovered this, but I don't think anything was already done around this, we should probably clean this up one day so only a single jquery file is in Magento's codebase?
jquery
subdir is really strange, introduced in MAGETWO-32494)Only the
jquery/jquery.min.js
file was marked to be excluded from the bundles, but the first one was still being included.I also removed some other files from the
view.xml
files of all themes:lib/web/mage/validation/dob-rule.js
in MAGETWO-32480lib/web/date-format-normalizer.js
in MAGETWO-31654Results after this PR:
mage/list.min.js
,mage/dropdown_old.min.js
,mage/zoom.min.js
,mage/translate-inline-vde.min.js
,mage/captcha.min.js
,mage/webapi.min.js
,mage/loader_old.min.js
,mage/requirejs/mixins.min.js
,mage/requirejs/static.min.js
,Magento_Catalog/js/zoom.min.js
,jquery/jquery.hoverIntent.min.js
,jquery/jquery-ui-1.9.2.min.js
,jquery/colorpicker/js/colorpicker.min.js
,requirejs/text.min.js
,requirejs/require.min.js
,Magento_Ui/js/form/element/file-uploader.min.js
,Magento_Ui/js/form/element/ui-select.min.js
,Magento_Ui/js/form/components/insert-listing.min.js
,Magento_Ui/js/form/components/insert.min.js
,Magento_Ui/js/lib/step-wizard.min.js
,Magento_Customer/js/zxcvbn.min.js
matchMedia.min.js
,mage/list.min.js
,mage/dropdowns.min.js
,mage/loader.min.js
,mage/dialog.min.js
,mage/zoom.min.js
,mage/translate-inline-vde.min.js
,mage/dataPost.min.js
,mage/terms.min.js
,mage/decorate.min.js
,mage/webapi.min.js
,mage/menu.min.js
,mage/common.min.js
,mage/deletable-item.min.js
,mage/sticky.min.js
,mage/item-table.min.js
,mage/dropdown.min.js
,mage/redirect-url.min.js
,mage/fieldset-controls.min.js
,mage/cookies.min.js
,mage/tooltip.min.js
,mage/toggle.min.js
,mage/gallery/gallery.min.js
,mage/requirejs/mixins.min.js
,mage/requirejs/static.min.js
,mage/adminhtml/varienLoader.min.js
,mage/adminhtml/tools.min.js
,mage/validation/validation.min.js
,jquery/jquery.parsequery.min.js
,jquery/jquery.mobile.custom.min.js
,requirejs/text.min.js
,requirejs/require.min.js
,varien/js.min.js
Be aware, this other open PR will shave of another 3.7 MB of the bundle size, so they will be a bit more reasonable.
Fixed Issues (if relevant)
This doesn't really fixes these issues, but is still related to them:
Manual testing scenarios (*)
Please test all JS functionality in blank, luma and backend themes and everything should keep working as expected when JS bundling is enabled (should be tested once with JS minification disabled, and also with it being enabled)
Maybe some performance testing could happen as well, see if this improves frontend performance somewhat?
Questions or comments
Also including @DrewML and @vzabaznov in this since they are also working hard on improving the frontend performance problems M2 has been suffering with.
Would it make sense for somebody to take a critical look to those excluded files from all themes, because maybe those lists can be optimised some more still?
The last time this happened seems to be around the time when 2.1.0 got released: MAGETWO-51628
Update: hmm, I'm wondering if we should keep jquery in the bundle by default? It looks like it gets loaded on every single page after some quick testing, so it probably was intended to be used in the bundle? I was confused due to the 2 different jquery files in the codebase where one of them was excluded, but that starts to makes sense now.
Contribution checklist (*)