At the moment, we use eslines when running build lint checks to only lint changed lines in the code base. This is less than ideal, as it often misses new errors that arise when we upgrade eslint or add / alter rules. Instead, we should get rid of all of the eslint errors and run eslint normally.
The following is a list of all of the eslint errors currently on master. If you'd like to fix a set, please make up a PR and add the PR# after the affected line.
When fixing the errors, there are three common strategies: 1) fix the offending code, 2) ignore the error with an // eslint-disable-* the-error instruction or 3) turn off the offending eslint rule entirely. 1 is preferred, 2 is understandable and 3 requires some broader discussion.
_This list was updated May 14 2018_
The following is a list of all of the eslint errors currently on master.
Does this omit some components or lint rules? I've seen eslint errors (from jsx-a11y) in my editor for foldable-card and bulk-select, but I don't see either of them on this list.
@ryelle nope, it was just the result of npx eslint . with a custom formatter that counts errors for file. It's been a while since I ran it, so perhaps they're new?
Please feel free to add anything that's missing!
@blowery I just ran eslint myself, only on client, and got this list (the a11y count is a subset of all errors, I was just curious). I'm not sure the best way to merge these lists, as mine is significantly longer 😞 Actually, I think since most of the associated issues have been merged, we might be OK to swap out the lists? It's just #24571 which hasn't landed yet.
@ryelle oof. I didn't run it with --ext js,jsx so i missed all the jsx files. :(
Thanks for bringing it up!
I'm working on merging the two lists now.
Actually, I think since most of the associated issues have been merged, we might be OK to swap out the lists?
I like that idea even better. :)
@ryelle list updated! Thanks again.
Here's a slightly different view of the errors:
❯ npx eslint -f bin/eslint-aggregate-formatter.js --ext js,jsx client
indent: 1788
wpcalypso/jsx-classname-namespace: 862
prefer-const: 647
react/no-string-refs: 257
valid-jsdoc: 247
jsx-a11y/click-events-have-key-events: 113
jsx-a11y/no-static-element-interactions: 103
max-len: 98
jsx-a11y/anchor-is-valid: 70
jsx-a11y/alt-text: 64
jsx-a11y/no-autofocus: 42
jsx-a11y/label-has-for: 41
react/prefer-es6-class: 39
wpcalypso/import-docblock: 24
no-duplicate-imports: 23
no-shadow: 23
wpcalypso/redux-no-bound-selectors: 15
jsx-a11y/mouse-events-have-key-events: 13
jsx-a11y/no-noninteractive-element-interactions: 13
no-console: 13
no-restricted-imports: 12
wpcalypso/i18n-no-collapsible-whitespace: 12
jest/no-disabled-tests: 11
jsx-a11y/heading-has-content: 11
wpcalypso/jsx-gridicon-size: 10
jest/no-identical-title: 7
jsx-a11y/no-noninteractive-tabindex: 7
jsx-a11y/tabindex-no-positive: 7
jsx-a11y/no-interactive-element-to-noninteractive-role: 6
react/no-did-update-set-state: 6
import/no-extraneous-dependencies: 5
jsx-a11y/no-onchange: 5
jsx-a11y/interactive-supports-focus: 4
jsx-a11y/role-supports-aria-props: 4
jsx-a11y/accessible-emoji: 3
jsx-a11y/iframe-has-title: 3
react/no-danger: 3
react/no-did-mount-set-state: 3
react/no-is-mounted: 3
jsx-a11y/media-has-caption: 2
no-multi-spaces: 2
no-var: 2
eqeqeq: 1
jsx-a11y/html-has-lang: 1
jsx-a11y/role-has-required-aria-props: 1
no-nested-ternary: 1
no-use-before-define: 1
operator-linebreak: 1
wpcalypso/i18n-no-variables: 1
total: 4630
here's the formatter:
/** @format */
const path = require( 'path' );
module.exports = function( results ) {
let totalErrors = 0;
const errorsPerRule = results.reduce( ( agg, current ) => {
current.messages.forEach( msg => {
agg.set( msg.ruleId, 1 + ( agg.get( msg.ruleId ) || 0 ) );
totalErrors++;
} );
return agg;
}, new Map() );
const pairs = Array.from( errorsPerRule );
pairs.sort( ( a, b ) => {
const countDiff = b[ 1 ] - a[ 1 ];
if ( countDiff !== 0 ) {
return countDiff;
}
return a[ 0 ].localeCompare( b[ 0 ] );
} );
pairs.forEach( pair => {
console.log( pair.join( ': ' ) );
} );
console.log( '\ntotal: ' + totalErrors );
};
Does an audit of eslint-disable comments fall under the scope of this issue? 😅
Here's where we stand now:
❯ npx eslint --ext js,jsx -f ../agg.js client
wpcalypso/jsx-classname-namespace: 839
prefer-const: 644
react/no-string-refs: 249
valid-jsdoc: 243
jsx-a11y/click-events-have-key-events: 106
jsx-a11y/no-static-element-interactions: 98
jsx-a11y/anchor-is-valid: 69
jsx-a11y/alt-text: 61
jsx-a11y/label-has-for: 41
jsx-a11y/no-autofocus: 40
react/prefer-es6-class: 39
wpcalypso/import-docblock: 24
no-shadow: 23
no-duplicate-imports: 22
wpcalypso/redux-no-bound-selectors: 15
jsx-a11y/mouse-events-have-key-events: 13
no-console: 13
no-restricted-imports: 12
wpcalypso/i18n-no-collapsible-whitespace: 12
jsx-a11y/no-noninteractive-element-interactions: 11
jsx-a11y/heading-has-content: 10
wpcalypso/jsx-gridicon-size: 10
jest/no-identical-title: 7
jsx-a11y/no-noninteractive-tabindex: 7
jsx-a11y/tabindex-no-positive: 7
jest/no-disabled-tests: 6
jsx-a11y/no-interactive-element-to-noninteractive-role: 6
react/no-did-update-set-state: 6
import/no-extraneous-dependencies: 5
jsx-a11y/no-onchange: 5
jsx-a11y/interactive-supports-focus: 4
jsx-a11y/role-supports-aria-props: 4
jsx-a11y/iframe-has-title: 3
react/no-danger: 3
react/no-did-mount-set-state: 3
react/no-is-mounted: 3
jsx-a11y/accessible-emoji: 2
jsx-a11y/media-has-caption: 2
no-var: 2
eqeqeq: 1
jsx-a11y/html-has-lang: 1
jsx-a11y/role-has-required-aria-props: 1
no-nested-ternary: 1
no-use-before-define: 1
wpcalypso/i18n-no-variables: 1
total: 2675
Updated the main list. It now includes the rules being violated and a count, along with a link to the offending file on master.
When #27089 lands, all of client/reader will pass :)
I wanted to see if we'd made progress since https://github.com/Automattic/wp-calypso/pull/27503 (see p4TIVU-99G-p2).
Using the formatter, here's the current report on master:
npm run lint:js -- -f bin/eslint-aggregate-formatter.js
no-shadow: 11377
no-var: 3182
valid-jsdoc: 861
no-unused-vars: 838
prefer-const: 360
no-undef: 332
new-cap: 331
eqeqeq: 238
strict: 202
no-nested-ternary: 191
react/no-deprecated: 112
no-use-before-define: 100
wpcalypso/jsx-classname-namespace: 49
wpcalypso/import-docblock: 48
no-console: 40
wpcalypso/i18n-no-variables: 20
no-cond-assign: 14
jest/no-alias-methods: 10
jest/no-disabled-tests: 6
jsx-a11y/anchor-is-valid: 6
jest/no-identical-title: 5
jsx-a11y/alt-text: 5
no-duplicate-imports: 5
import/no-nodejs-modules: 4
jsx-a11y/mouse-events-have-key-events: 4
no-process-exit: 4
wpcalypso/import-no-redux-combine-reducers: 4
jest/valid-describe: 3
jsx-a11y/click-events-have-key-events: 3
jsx-a11y/label-has-associated-control: 3
jsx-a11y/no-static-element-interactions: 3
react/jsx-no-target-blank: 3
dot-notation: 2
import/no-extraneous-dependencies: 2
jest/no-test-prefixes: 1
jest/valid-expect-in-promise: 1
jsx-a11y/no-autofocus: 1
wpcalypso/redux-no-bound-selectors: 1
total: 18371
For reference, I ran against https://github.com/Automattic/wp-calypso/commit/6dc51e9edc64fd30ef9dc86bc28d0cb92dfe9e5c where we started making lint fixes "compulsory", we were at a total of 18373 there.
We seem to have fixed 2 lint errors?! Can that be right?
Full report from 6dc51e9edc64fd30ef9dc86bc28d0cb92dfe9e5c
no-shadow: 11371
no-var: 3182
valid-jsdoc: 867
no-unused-vars: 831
prefer-const: 358
new-cap: 331
no-undef: 327
eqeqeq: 238
strict: 202
no-nested-ternary: 191
react/no-deprecated: 114
no-use-before-define: 100
wpcalypso/jsx-classname-namespace: 49
wpcalypso/import-docblock: 48
no-console: 42
jest/no-disabled-tests: 36
wpcalypso/i18n-no-variables: 20
no-cond-assign: 14
jsx-a11y/alt-text: 6
jsx-a11y/anchor-is-valid: 6
jest/no-identical-title: 5
no-duplicate-imports: 5
jsx-a11y/mouse-events-have-key-events: 4
no-process-exit: 4
wpcalypso/import-no-redux-combine-reducers: 4
jsx-a11y/click-events-have-key-events: 3
jsx-a11y/label-has-associated-control: 3
jsx-a11y/no-static-element-interactions: 3
react/jsx-no-target-blank: 3
dot-notation: 2
wpcalypso/redux-no-bound-selectors: 2
import/no-nodejs-modules: 1
jsx-a11y/no-autofocus: 1
total: 18373
And a diff of the reports
--- a.txt 2018-11-22 11:32:41.000000000 +0100
+++ b.txt 2018-11-22 11:32:56.000000000 +0100
@@ -1,40 +1,35 @@
-no-shadow: 11377
+no-shadow: 11371
no-var: 3182
-valid-jsdoc: 861
-no-unused-vars: 838
-prefer-const: 360
-no-undef: 332
+valid-jsdoc: 867
+no-unused-vars: 831
+prefer-const: 358
new-cap: 331
+no-undef: 327
eqeqeq: 238
strict: 202
no-nested-ternary: 191
-react/no-deprecated: 112
+react/no-deprecated: 114
no-use-before-define: 100
wpcalypso/jsx-classname-namespace: 49
wpcalypso/import-docblock: 48
-no-console: 40
+no-console: 42
+jest/no-disabled-tests: 36
wpcalypso/i18n-no-variables: 20
no-cond-assign: 14
-jest/no-alias-methods: 10
-jest/no-disabled-tests: 6
+jsx-a11y/alt-text: 6
jsx-a11y/anchor-is-valid: 6
jest/no-identical-title: 5
-jsx-a11y/alt-text: 5
no-duplicate-imports: 5
-import/no-nodejs-modules: 4
jsx-a11y/mouse-events-have-key-events: 4
no-process-exit: 4
wpcalypso/import-no-redux-combine-reducers: 4
-jest/valid-describe: 3
jsx-a11y/click-events-have-key-events: 3
jsx-a11y/label-has-associated-control: 3
jsx-a11y/no-static-element-interactions: 3
react/jsx-no-target-blank: 3
dot-notation: 2
-import/no-extraneous-dependencies: 2
-jest/no-test-prefixes: 1
-jest/valid-expect-in-promise: 1
+wpcalypso/redux-no-bound-selectors: 2
+import/no-nodejs-modules: 1
jsx-a11y/no-autofocus: 1
-wpcalypso/redux-no-bound-selectors: 1
-total: 18371
\ No newline at end of file
+total: 18373
\ No newline at end of file
We seem to have fixed 2 lint errors?! Can that be right?
That's... odd. A bunch of specific errors actually got worse...
Marked all /site-settings/site-settings/action-panel errors as done. action-panel was moved to being a component in https://github.com/Automattic/wp-calypso/pull/24860 and errors are no longer seen.
Thanks @arunsathiya!
I think I'm going to close this issue, as it's ancient and too unwieldy to really track well.
Fair point! But, I believe patches are still welcome. 🙃
@arunsathiya oh yes indeed! all patches welcome and appreciated.