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

zxcvbn.js included in merged.js #14357

Closed
cannycookie opened this issue Mar 25, 2018 · 10 comments
Closed

zxcvbn.js included in merged.js #14357

cannycookie opened this issue Mar 25, 2018 · 10 comments
Labels
Component: Theme Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release

Comments

@cannycookie
Copy link

zxcvbn.js is not included in our theme, but when js bundling and merging is enabled this 850Kb file is added to the merged script.

Preconditions

  1. Magento 2.2.1
  2. Custom Theme

Steps to reproduce

  1. Enable a custom theme with no reference to password-strength-indicator or zxcvbn.js
  2. create override for magento-customer/view/frontend/requirejs-config.js and remove
    passwordStrengthIndicator: 'Magento_Customer/js/password-strength-indicator', zxcvbn: 'Magento_Customer/js/zxcvbn',
  3. Enable JS Bundling and merging
  4. Enable production mode

Expected result

  1. zxcvbn.js should not be included

Actual result

  1. Search minified script and find 122 instances of fuck
@magento-engcom-team magento-engcom-team added the Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed label Mar 25, 2018
@ghost ghost self-assigned this Aug 8, 2018
@ghost ghost added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release Component: Theme labels Aug 8, 2018
@ghost ghost removed their assignment Aug 8, 2018
@ghost
Copy link

ghost commented Aug 8, 2018

@cannycookie, thank you for your report.
We've acknowledged the issue and added to our backlog.

@DanielRuf
Copy link
Contributor

Afaik this is part of login and others things which use it for calculating the password strength.

You should be able to remove the js file according to the docs.

@bery
Copy link

bery commented Aug 9, 2018

I did some testing and I was not able to remove the file as it is merged via require js config. When merging is disabled, all works fine, yet when merging is enabled, the library is included on all pages.

@DanielRuf
Copy link
Contributor

You have to explicitely exclude it from bundling (which is done with all files which are loaded with requireJS):

https://devdocs.magento.com/guides/v2.2/frontend-dev-guide/themes/js-bundling.html

See https://github.com/magento/magento2/blob/c896f2c07c4bf61481aad75bab2467832594b546/app/design/frontend/Magento/blank/etc/view.xml for reference.

@RaphaelBronsveld
Copy link

RaphaelBronsveld commented Nov 23, 2018

Hi,

It seems that excluding via the view.xml file only works for directories. We copied the exclude node from the Blank theme to our custom theme, but the libraries are still in the bundled+merged+minified js.

For example:

  • "jquery.min.js"
  • "moment-timezone-with-data.js"
  • "moment.js"
  • "Magento_Customer/js/zxcvbn.js"

Are still in the bundled javascript.

This is on Magento 2.2.5.

Anyone else experiencing this?

@gwharton
Copy link
Contributor

gwharton commented Dec 2, 2018

Adding this to the exclude list in view.xml gets rid of moment-timezone-with-data.min.js

<item type="file">Lib::moment-timezone-with-data.js</item>

and adding

<item type="file">Magento_Customer::js/zxcvbn.min.js</item>

gets rid of zxcvbn (note including the .min.

@RaphaelBronsveld
Copy link

Adding this to the exclude list in view.xml gets rid of moment-timezone-with-data.min.js

<item type="file">Lib::moment-timezone-with-data.js</item>

and adding

<item type="file">Magento_Customer::js/zxcvbn.min.js</item>

gets rid of zxcvbn (note including the .min.

Based on the https://devdocs.magento.com/guides/v2.2/performance-best-practices/configuration.html#bundling-tips documentation, I disabled merging. Only minify + bundling is enabled.

The zxcvbn (with min) worked for me. The timezone data without did not. I included .min in the exclude and it worked.

Same with <item type="file">Lib::jquery/jquery-ui-1.9.2.js</item>, this doesn't work for me, but <item type="file">Lib::jquery/jquery-ui-1.9.2.min.js</item> does.

So I assumed, adding .min. will fix this for several js files, but..

Blank luma theme adds this: <item type="file">Lib::jquery/jquery.min.js</item>. And this is still included in one of the bundles?

@adifucan
Copy link
Contributor

adifucan commented Aug 7, 2019

Hi @cannycookie!
I created a custom theme for Magento 2.3.-develop. This theme extends luma theme. It has its custom theme requirejs-config without passwordStrengthIndicator & zxcvbn. I enabled JS merging and bundling and switched to production mode.
And I could not find zxcvbn library in bundles and in merge file either. zxcvbn library is requested separately/asynchronously when I register custom, edit customer's password or reset customer's password.
It's because zxcvbn library is excluded from bundles in app/design/frontend/Magento/luma/etc/view.xml in exclude node.
So, may be this bug is already fixed in 2.3-develop. Or maybe your custom theme does not extend blank or luma themes and you do not specify zxcvbn library in exclude node in your app/design/frontend/Vendor/theme-name/etc/view.xml file. exclude node in view.xml defines the list of resources that should not be bundled.

@adifucan
Copy link
Contributor

adifucan commented Aug 8, 2019

Please, reopen this issue with more details if it still occurs.

@magento-engcom-team
Copy link
Contributor

Hi @cannycookie. Thank you for your report.
The issue has been fixed in #24506 by @hostep in 2.3-develop branch
Related commit(s):

The fix will be available with the upcoming 2.3.4 release.

@magento-engcom-team magento-engcom-team added the Fixed in 2.3.x The issue has been fixed in 2.3 release line label Sep 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Theme Fixed in 2.3.x The issue has been fixed in 2.3 release line Issue: Clear Description Gate 2 Passed. Manual verification of the issue description passed Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: Format is valid Gate 1 Passed. Automatic verification of issue format passed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

No branches or pull requests

7 participants