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

ESLint errors fix #9091

Merged
merged 3 commits into from
May 16, 2017
Merged

ESLint errors fix #9091

merged 3 commits into from
May 16, 2017

Conversation

Igloczek
Copy link
Contributor

@Igloczek Igloczek commented Apr 1, 2017

It's part of #9089.

This PR contains only safe changes generated using --fix flag for ESLint.
I manually excluded all of 3rd party code from lib/web, overwriting them after fixing process. Unfortunatly 3rd party code is mixed with Magento code within lib/web, so there isn't a quick way to use eslint against only custom code, without blacklisting 3rd party manually.

Manual testing scenarios

  1. Install ESLint globally (yarn global add eslint / npm install -g eslint)
  2. eslint -c dev/tests/static/testsuite/Magento/Test/Js/_files/eslint/.eslintrc --fix lib/**/*.js
  3. eslint -c dev/tests/static/testsuite/Magento/Test/Js/_files/eslint/.eslintrc --fix app/**/*.js

Copy link
Contributor

@omiroshnichenko omiroshnichenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don`t have practice to modify 3rd party libraries, for static test we have blacklist that exludes these files. Most of your fixes is in libraries. Can you please revert this changes?

@Igloczek
Copy link
Contributor Author

Looks like I have to redo everything, but I have some concerns about your current blacklist and whitelist content. IMO it doesn't cover all of your custom code, that's why tests currently are passing.
First lines of blacklist cleairly contains your custom modules, not 3rd party libs:

app/code/Magento/Authorizenet/view/adminhtml/web/js/direct-post.js
app/code/Magento/Catalog/view/adminhtml/web/catalog/product/composite/configure.js
app/code/Magento/ConfigurableProduct/view/adminhtml/web/js/options/price-type-handler.js
app/code/Magento/Rule/view/adminhtml/web/rules.js
app/code/Magento/Sales/view/adminhtml/web/order/create/giftmessage.js
app/code/Magento/Sales/view/adminhtml/web/order/create/scripts.js
app/code/Magento/Shipping/view/adminhtml/web/order/packaging.js

Varien code also is your custom solution, but is blacklisted:

lib/web/mage/adminhtml/tools.js
lib/web/mage/adminhtml/varienLoader.js

Also when we take a look on whitelist, few dirs are missing, i.e. we have lib/web/mage/**/*.js, but the custom code can be found inlib/web too:

  • lib/web/css/docs/source/js/dropdown.js
  • lib/web/extjs/defaults.js
  • lib/web/less/config.less.js
  • lib/web/fotorama (it's 3rd party, but customized 🙄 )
  • lib/web/magnifier
  • lib/web/varien

@omiroshnichenko
Copy link
Contributor

@Igloczek About blacklisted files(not 3rd party), we already have internal ticket to refactor these files, because most of them didn`t support strict mode.

About files that you change, please, exclude these files:

  • app/code/Magento/Customer/view/frontend/web/js/zxcvbn.js
  • app/code/Magento/Swagger/view/frontend/web/swagger-ui/js/lib/backbone-min.js
  • app/code/Magento/Swagger/view/frontend/web/swagger-ui/js/lib/handlebars-2.0.0.js
  • app/code/Magento/Swagger/view/frontend/web/swagger-ui/js/lib/highlight.7.3.pack.js
  • app/code/Magento/Swagger/view/frontend/web/swagger-ui/js/lib/jquery-1.8.0.min.js
  • app/code/Magento/Swagger/view/frontend/web/swagger-ui/js/lib/jquery.ba-bbq.min.js
  • app/code/Magento/Swagger/view/frontend/web/swagger-ui/js/lib/jquery.slideto.min.js
  • app/code/Magento/Swagger/view/frontend/web/swagger-ui/js/lib/jquery.wiggle.min.js
  • app/code/Magento/Swagger/view/frontend/web/swagger-ui/js/lib/marked.js
  • app/code/Magento/Swagger/view/frontend/web/swagger-ui/js/lib/swagger-oauth.js
  • app/code/Magento/Swagger/view/frontend/web/swagger-ui/js/lib/underscore-min.js
  • lib/web/extjs/ext-tree-checkbox.js
  • lib/web/es6-collections.js
  • lib/web/fotorama/fotorama.js
  • lib/web/fotorama/fotorama.min.js
  • lib/web/jquery/colorpicker/js/colorpicker.js
  • lib/web/jquery/editableMultiselect/js/jquery.editable.js
  • lib/web/jquery/editableMultiselect/js/jquery.multiselect.js
  • lib/web/jquery/fileUploader/canvas-to-blob.js
  • lib/web/jquery/fileUploader/cors/jquery.postmessage-transport.js
  • lib/web/jquery/fileUploader/cors/jquery.xdr-transport.js
  • lib/web/jquery/fileUploader/jquery.fileupload-fp.js
  • lib/web/jquery/fileUploader/jquery.fileupload-ui.js
  • lib/web/jquery/fileUploader/jquery.fileupload.js
  • lib/web/jquery/fileUploader/jquery.iframe-transport.js
  • lib/web/jquery/fileUploader/load-image.js
  • lib/web/jquery/fileUploader/main.js
  • lib/web/jquery/fileUploader/vendor/jquery.ui.widget.js
  • lib/web/jquery/jquery-migrate.js
  • lib/web/jquery/jquery-ui-1.9.2.js
  • lib/web/jquery/jquery-ui-timepicker-addon.js
  • lib/web/jquery/jquery-ui.js
  • lib/web/jquery/jquery.ba-hashchange.min.js
  • lib/web/jquery/jquery.cookie.js
  • lib/web/jquery/jquery.details.js
  • lib/web/jquery/jquery.hoverIntent.js
  • lib/web/jquery/jquery.metadata.js
  • lib/web/jquery/jquery.min.js
  • lib/web/jquery/jquery.mobile.custom.js
  • lib/web/jquery/jquery.storageapi.min.js
  • lib/web/jquery/jquery.tabs.js
  • lib/web/jquery/jquery.validate.js
  • lib/web/jquery/jstree/jquery.jstree.js
  • lib/web/knockoutjs/knockout-es5.js
  • lib/web/knockoutjs/knockout-fast-foreach.js
  • lib/web/knockoutjs/knockout-repeat.js
  • lib/web/knockoutjs/knockout.js

Most of them not under Magento license.
Thanks.

@Igloczek
Copy link
Contributor Author

Igloczek commented May 4, 2017

@omiroshnichenko I had to redo everything. I updated blacklist and whitelists to cover more files and clearly mark why some of them are blacklisted, b/c need serious refactoring.

Also instead of blacklisting whole files, I added some ESLint and JSCS ignores inside files.

BTW. JSCS has merged with ESLint, are you open to updates in this area?

@omiroshnichenko
Copy link
Contributor

@Igloczek Looks good. About linters merging, I made small investigation about that and I think we can make update in nearest future.

@ishakhsuvarov ishakhsuvarov added this to the May 2017 milestone May 12, 2017
@magento-team magento-team merged commit 05ea5cd into magento:develop May 16, 2017
magento-team pushed a commit that referenced this pull request May 16, 2017
@magento-team
Copy link
Contributor

@Igloczek Thank you for the contribution

@Igloczek Igloczek deleted the eslint-errors-fix branch May 28, 2017 23:03
magento-team pushed a commit that referenced this pull request Jun 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants