Usage of outdated ESLint configuration

[swissspidy]

Bug Description

While working on #163, I noticed that ESLint does not use the recommended and most up-to-date @wordpress/eslint-plugin to help adhere to WordPress coding standards. Instead, it uses the outdated and deprecated eslint-config-wordpress package.

I remember flagging this in the past, but it seems this has been forgotten.

Also, npm run lint:js does not lint any test files or config files. Personally, I think the code style should be adhered everywhere throughout the project, not just in assets/js.

You can easily see that in some of the configuration files which have inconsistent indentation (example: https://github.com/google/site-kit-wp/pull/163/files#diff-df39304d828831c44a2b9f38cd45289c) and things like that.

To achieve this, an .eslintignore file can be added to make sure only relevant project files are run through ESLint, and not things like node_modules or the dist folder.

This would be a good opportunity to check some of the rule exceptions in the ESLint configuration as well.

For comparison, the ESLint setup in the AMP plugin uses that ESLint plugin as well and is super simple:

https://github.com/ampproject/amp-wp/blob/fd7a5adf5f7e3b74c3b8c386b220249237cc34c4/.eslintrc

https://github.com/ampproject/amp-wp/blob/fd7a5adf5f7e3b74c3b8c386b220249237cc34c4/.eslintignore

---

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

• Site Kit should use the latest version of the @wordpress/eslint-plugin package for linting JavaScript.
• The eslint-config-wordpress package should no longer be used.
• .eslintrc should extend the @wordpress/eslint-plugin configuration and not contain any custom rule overrides (as it does now).
• An .eslintignore file should be added to ignore vendor / build directories from linting.
• The npm run lint:js command should lint all JavaScript files in the project, not only those in the assets directory. The only exception are the "automatically" ignored ones from .eslintignore.
• All JavaScript code should be rule-compliant with the new configuration (most things can probably be auto-fixed).

Implementation Brief

• Add @wordpress/eslint-plugin to the devDependencies in package.json
• Remove babel-eslint, eslint-config-standard, eslint-config-wordpress, eslint-plugin-node, eslint-plugin-promise, eslint-plugin-react, eslint-plugin-standard from devDependencies in package.json.
  These are either covered by @wordpress/eslint-plugin or currently not used at all and thus .
• Adjust .eslintrc to extend only plugin:@wordpress/eslint-plugin/recommended, and plugin:jest/recommended.
• Remove all existing custom rules in .eslintrc
• Remove parser option from .eslintrc (already covered by @wordpress/eslint-plugin)
• Add .eslintignore file to ignore vendor and build directories from linting and update npm run lint:js to lint all JS files in plugin by only ignoring the ones added to .eslintignore
• Fix any lint warnings and errors after new configuration.

Changelog entry

• N/A

[swissspidy]

I made a slight modification to be more explicit about what kind of changes need to be done to the ESLint configuration.

The last part, Fix any lint warnings and errors after new configuration. might be a bit too vague. I guess most issues will only pop up once the actual changes are done, but here's something that comes to mind:

1. For the linting to be as effective as possible, this would ideally also involve changing things like const { withFilters } = wp.components to use import { withFilters } from '@wordpress/components; in order for the dependency grouping rule (part of @wordpress/eslint-plugin) to work properly.
2. There need to be adjusments made to the env and globals options in the ESLint configuration.
   For example, for E2E test files, page need to be added as a global.

[aaemnnosttv]

Updating the estimate on this one as it appears to require more manual updates than expected. Also, there was a problem while upgrading to the new eslint package which required a bit more time as well.

[swissspidy]

Yeah S is more accurate here I think.

Regarding eslint, worth noting that there seems to be a bug in 6.2.x in combination with babel-eslint that shows some false positives.

[aaemnnosttv]

Thanks for the heads up! Is there an issue you can link me to for more info/context?

[aaemnnosttv]

For the linting to be as effective as possible, this would ideally also involve changing things like const { withFilters } = wp.components to use import { withFilters } from '@wordpress/components; in order for the dependency grouping rule (part of @wordpress/eslint-plugin) to work properly.

@swissspidy is this something that we should consider is an amendment to the acceptance criteria (i.e. required) or is this more of a nice to have - if it fits in the time of the estimate and if not we add ignores for any remaining and handle those in a subsequent ticket?

[swissspidy]

Is there an issue you can link me to for more info/context?

eslint/eslint#12117
babel/babel-eslint#791

Might be possible that this project here is not affected by this though.

is this something that we should consider is an amendment to the acceptance criteria (i.e. required) or is this more of a nice to have - if it fits in the time of the estimate and if not we add ignores for any remaining and handle those in a subsequent ticket?

It's more of a nice to have if it fits in the time. No ignores should be needed otherwise, as the rule simply would not apply.

[aaemnnosttv]

Update:

• Qunit file has been added to eslintignore as there does not appear to be a good way to apply a "downgraded" ruleset to a subset of files without fragmenting the config into multiple files
• eslint has been left at the 5.x version due to false positives in the latest v6 (ESLint 6.2.0 + babel-eslint + no-unused-vars false positive with for-in loop eslint/eslint#12117)
  e.g.
  

  site-kit-wp/assets/js/util/cache-data.js

  Lines 61 to 63 in 23b918f

  	// Go thru each endpoint from the predefined list.
  	for ( let endpoint of endpoints ) {
  	console.log( 'Caching %s', endpoint ); // eslint-disable-line no-console

  
  

  site-kit-wp/assets/js/util/cache-data.js

  Line 106 in 23b918f

  for ( let scriptName of scriptsToMatch ) {

[swissspidy]

has been left at the 5.x version due to false positives in the latest v6

Why not upgrade to 6.2.0 then instead of latest (6.2.1)? I think ESLint 6 is the version the WP ESLint plugin is tested with and works best.

[aaemnnosttv]

The issue exists in 6.2.0 as well as reported in eslint/eslint#12117. I'll try 6.1.0 which is said to not exhibit the problem.

[swissspidy]

Ah, right, that's what I meant.

[aaemnnosttv]

OK 6.1.0 is working just fine. Pushing the update now, can you review as soon as the build finishes @swissspidy?

[aaemnnosttv]

Took a little bit longer to work out the kinks as I didn't realize some of the other parts were failing, but it should be all good now! 😄

[circlecube]

QA pass. All acceptance criteria checks out. Running npm run link:js gives 0 errors and 75 warnings, all look to be related to JSDocs missing bits, mostly return descriptions.
✖ 75 problems (0 errors, 75 warnings)