Update ESLint configuration

[fredden]

Description

The ESLint configuration currently in use was last updated four years ago. A lot has changed in the JavaScript since then. Let's update the configuration to match today's standards.

Related Pull Requests

Fixed Issues

None

Manual testing scenarios

This is a coding standards change and does not make any changes to any functionality within the platform. Therefore no testing of Magento itself is required.

Questions or comments

I expect this pull request to spark some conversation. Many of the settings here have come from the JavaScript Standard Style, although some adaptations have been made to better fit with Magento's existing style guide (eg, four spaces for indentation). I have intentionally kept all existing settings unchanged.

I'm happy to receive feedback here, but may need to get someone from Magento to give a deciding vote / official stance if consensus cannot be amicably achieved.

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)

Resolved issues:

1. resolves [Issue] Update ESLint configuration #32375: Update ESLint configuration

[m2-assistant]

Hi @fredden. Thank you for your contribution
Here are 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.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

• @magento run all tests - run or re-run all required tests against the PR changes
• @magento run <test-build(s)> - run or re-run specific test build(s)
  For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

1. Database Compare
2. Functional Tests CE
3. Functional Tests EE,
4. Functional Tests B2B
5. Integration Tests
6. Magento Health Index
7. Sample Data Tests CE
8. Sample Data Tests EE
9. Sample Data Tests B2B
10. Static Tests
11. Unit Tests
12. WebAPI Tests
13. Semantic Version Checker

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

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

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

[mrtuvn]

Mostly all magento js write in es5+. Get benefits form newer js generation by add syntax such as arrow function or spared operator
without babel may not work in some browser not support
imho i think we also add babel in package.json.sample in repo root with @babel/core and @babel/preset-env

[gabrieldagama]

@magento create issue

[mrtuvn]

@magento run Static Tests, Functional Tests CE, Functional Tests EE, Functional Tests B2B

[mrtuvn]

no-var may cause some errors since i see lot of js code still use var ?

[fredden]

https://eslint.org/docs/rules/no-var

The idea here is to configure ESLint to encourage use of modern JavaScript. Yes, var still works but let and const are preferred.

[fredden]

Perhaps the testsuite should be changed to only fail ESLint checks on GitHub for files that are being modified, like it already does for PHP_CodeSniffer. This would mean we can get this configuration merged in and then migrate the existing JavaScript gradually. It'd be messy to try to do it all in one go here, especially as there are third party libraries and minified JavaScript files in the repository currently.

@gabrieldagama or @mrtuvn, do you know how one might go about making such a change? I don't recall seeing code for the GitHub testsuite integration. Is there another repository to which I can be given access so that I can make this change?

[fredden]

without babel may not work in some browser not support
imho i think we also add babel in package.json.sample in repo root with @babel/core and @babel/preset-env

I don't have a problem adding Babel to package.json.sample. Please let me know if you think that should be in this pull request. I don't think a transpiler is required for any of the browsers on the currently supported list though.

If a particular website is trying to support an older / unsupported browser, then a transpiler makes sense along with various polyfills and library compatibility layers. Finding these and maintaining a list thereof seems out of scope to me.

Perhaps it would be better to have a note in the documentation advising the existence of polyfills and transpilers. That would allow the codebase to respect its list of supported browsers, and provide information for those who wish to extend that support without the burden of (this project) supporting (a list of tools for compatibility with) unsupported browsers.

[mrtuvn]

thank you i think we no need babel at this time! Let's focus on other archievement and fix static fails first because i think a lot of files in codebase will not following standards

[mrtuvn]

@magento run Static Tests

[fredden]

@gabrieldagama, can I please get access to https://github.com/magento-commerce/magento2-infrastructure/tree/2.4-develop or wherever the Static Tests configuration is to make the change suggested here?

[gabrieldagama]

Hi @fredden, I'm afraid we won't be able to provide access to that repository, which is private and internal repo. However, do you work for an Adobe/Magento partner? If so access to https://github.com/magento/partners-magento2-infrastructure may be provided.

Otherwise, we can proceed with this PR later. It may take a while because of its priority.

[fredden]

Yes, I currently work for a Magento Partner. How do I go about requesting access to https://github.com/magento/partners-magento2-infrastructure?

[gabrieldagama]

As discussed with @fredden on slack, that repo is actually not accessible through our partners program, so in order to proceed with this PR a member of our team has to work on this.

[mrtuvn]

@sivaschenko @fascinosum added label here( maybe adjust priority) but we still need extra works on this. A lot of js static fails with new changes in eslint rules. Imho i think this updates neccessary for code base in long terms

[fredden]

I'm still willing to make the proposed changes to the test-suite to facilitate a smooth / easy roll-out of this change. Is someone able to grant me access to the relevant repositories?

[sivaschenko]

@mrtuvn this PR is not related to the 3rd party or infrastructure dependencies of Magento (that are the platform), so I have removed the Platform Health project label

[fredden]

@sivaschenko is there anything I can do to help progress this?

[magento-automated-testing]

The requested builds are added to the queue. You should be able to see them here within a few minutes. Please re-request them if they don't show in a reasonable amount of time.

[mrtuvn]

Any tool auto fix with your new config eslint ? Since a think a lot file js in code base will false after update new config

[fredden]

I'd like to get the static tests for JavaScript on GitHub changed so that it will only lint files that have been changed in the pull request, rather than every file in the code-base. This is how the static tests for PHP work at present, so there's no change in policy. As a side benefit, the static tests will use less resource and run quicker. (Other tests (eg Functional, Integration, Unit) tests won't be affected.)

The main reason for this suggestion/change is so we don't have to modify all JavaScript files within one pull request. Such a pull request would be unreasonably large and require significant testing before being accepted, and will very likely have many conflicts which would be time consuming to resolve. Given the long length of time any change requires to go through, this is infeasible.

This approach (update static tests to treat JavaScript & PHP equally) allows us to work through the JavaScript files in a sensible & feasible way. Files that are changed should be brought in line with the current standards (which is how PHP files are being handled presently). Over time we will have more and more files matching the current standards. We can plan a project to update all files if desired. In my opinion this should be handled separately from updating the standards.

[fredden]

@sivaschenko is there anything I can do to help progress this?

[sivaschenko]

@fredden @mrtuvn this pull request was prioritized as P4 for magento2, so it will not be processed soon.

However, we are planning to extract eslint tests and configuration to magento-coding-standard configuration, this pull request will be considered in scope of this work.

[fredden]

Thanks for the update @sivaschenko. I can see that magento/magento-coding-standard#262 has been opened. Should I recreate this pull request in that repository?

[sivaschenko]

@fredden I think we might need to do this change in several stages:

1. Move eslint tests to magento-coding-standard
2. Include eslint tests from magento-coding-standard in magento2 repository (and remove the legacy mechanism)
3. Implement eslint testing for only changed files on pull request
4. Update the ruleset

Only the rules that will not fail Static tests build on the magento2 codebase can be added earlier.

[fredden]

This has been superseded by magento/magento-coding-standard#347.