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

Fixes excluding js files from bundles when minifying is enabled #24506

Conversation

hostep
Copy link
Contributor

@hostep hostep commented Sep 7, 2019

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:

  • files are put in pub/static/ per theme and locale
  • when JS minification is enabled, all non-already minified files are minified and get a .min.js filename
  • when JS bundling is enabled it will first make a list of all the files in pub/static/ and then match those filenames against the files of files in the view.xml to determine if they should be included or excluded from the bundle

The 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?

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:

  • The minified files in there shouldn't have been excluded as that should happen automatically (this is what this PR is doing)
  • The tiny_mce subdirs were not necessary to be explicitly specified, because the main directory was already marked to be excluded
  • Other files were removed a long time ago:

Results after this PR:

Theme Files no longer in bundle Bundle size before PR Bundle size after PR
Blank & Luma 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 7.0MB 6.5MB
Backend 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 7.2MB 7.1MB

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 (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Sep 7, 2019

Hi @hostep. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@magento-engcom-team
Copy link
Contributor

Hi @VladimirZaets, thank you for the review.
ENGCOM-5815 has been created to process this Pull Request
✳️ @VladimirZaets, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@hostep
Copy link
Contributor Author

hostep commented Sep 9, 2019

@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?

@DrewML
Copy link

DrewML commented Sep 9, 2019

@hostep thanks for working on this. You'll definitely want jQuery in the main bundle. All core functionality (including things like mage-init) relies on it, so module execution on the client-side won't really start happening until jQuery has defined itself on the page.

@hostep hostep force-pushed the fixes-excluding-js-files-from-bundles-when-minifying-is-enabled branch from f5aa1cd to cbe25f4 Compare September 9, 2019 17:46
@hostep
Copy link
Contributor Author

hostep commented Sep 9, 2019

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.

@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

@magento-engcom-team magento-engcom-team merged commit cbe25f4 into magento:2.3-develop Sep 14, 2019
@m2-assistant
Copy link

m2-assistant bot commented Sep 14, 2019

Hi @hostep, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@magento-engcom-team magento-engcom-team added this to the Release: 2.3.4 milestone Sep 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants