Task: Fix ESlint Errors (tracking issue)

scinos commented 2 years ago

Details

Current list of errors (updated to commit 6a130575c5759ba153f1310e9de36cb4629a19a4)

Rule	Errors	Disabled	Autofix	Issue
wpcalypso/jsx-classname-namespace	273	308
jsx-a11y/anchor-is-valid	24	21
jsx-a11y/alt-text	23	5
jsx-a11y/click-events-have-key-events	19	31
jsx-a11y/no-static-element-interactions	18	22
jsdoc/no-undefined-types	17	5		#56607
no-restricted-imports	15	15
no-console	14	100
no-shadow	10	9
no-unused-vars	7	18
eqeqeq	6	3	✅
react/no-danger	5	25
no-case-declarations	5	19
import/order	11	4	✅
wpcalypso/jsx-gridicon-size	4	19
import/named	4	3		#56981
wpcalypso/redux-no-bound-selectors	3	17
wpcalypso/i18n-ellipsis	3	0	✅
prettier/prettier	3	0	✅
jsx-a11y/heading-has-content	3	5
jest/no-export	3	2
@typescript-eslint/no-use-before-define	3	21
react/jsx-no-target-blank	2	4	✅
no-undef	2	24
no-nested-ternary	2	8
no-global-assign	2	2
jsx-a11y/no-noninteractive-element-interactions	2	7
jsx-a11y/no-autofocus	2	41
jsx-a11y/iframe-has-title	2	1
wpcalyspo/jsx-classname-namespace	1	0
wpcalypso/i18n-mismatched-placeholders	1	1
react/no-did-update-set-state	1	4
jsx-a11y/role-supports-aria-props	1	0
jsx-a11y/html-has-lang	1	0
import/no-nodejs-modules	1	36

Fixed errors

These errors have been already fixed. The list is here for documentation purposes:

Rule	Errors	Disabled	Autofix	Issue
jsdoc/require-param	76	4	✅	#56577
jsdoc/no-undefined-types	65	5		#56607
jsdoc/check-param-names	50	0		#56604
react/no-string-refs	32	3		#56719
@typescript-eslint/ban-types	21	7		#56612
jsdoc/require-param-description	19	0		#56577
jest/expect-expect	17	17		#56698
jsdoc/check-tag-names	16	15		#56605
jest/no-conditional-expect	9	4		#56721
jsdoc/newline-after-description	6	0	✅	#56606
jsdoc/require-returns-description	5	0		#56577
jsdoc/check-values	2	1		#56609
jsdoc/check-alignment	2	0	✅	#56580
jest/no-identical-title	2	1		#56698
jest/valid-title	1	3		#56698
jsx-a11y/accessible-emoji	2	0

Checklist

No response

Given that we don't generate external docs from the source code, and I assume that most devs use VSCode (or an editor with very similar features regarding code intellisense), I based this analysis on the usefulness of JSDoc @param for VSCode in two scenarios: code intellisense when typing and when mouse hover the function.

With no @param documented

On hover	On type

Note how VSCode correctly infers the parameters name and types them as any.

With just `@param <name>`	On hover	On type

Intellisense when typing is exactly the same. On hover it adds the parameter names after the function description, but that info was already visible in the function signature.

With `@param <type> <name>`	On hover	On type

Now it shows the types in the function signature in both cases, but it doesn't in param list after the function description

With `@param <type> <name> <description>`	On hover	On type

Shows the description in both cases.

Given the results above, I think adding just @param <name> doesn't provide any value. As such, I think we should disable jsdoc/require-param but keep jsdoc/check-param-names (don't force adding @param, but if a @param is present, ensure it matches the actual param name).

Enforcing param description is also counter productive. In many cases the param information is available in the function description, can be easily inferred by the name or there is simply no relevant description. Enforcing it will likely cause lots of useless @param <string> id - The id The same applies to jsdoc/require-returns-description

Now only @param <type> <name> is left. We can enforce it by enabling jsdoc/require-param-type. While arguably it adds some value, the reality is we'll have lots errors (348 errors) if we enable it. Fixing them is not a trivial task.

Type checks won't be enforced (at least my VSCode config ignores any type discrepancy). Given that we already have infrastructure in place to transpile TypeScript in most packages (example: client/ is already a TypeScript + JavaScript project), if we think types are important enough to be mandatory, maybe we should convert those files to TypeScript.

In short, I propose:

Disable jsdoc/require-param
Disable jsdoc/require-returns-description
Disable jsdoc/require-param-description
Keep jsdoc/no-undefined-types
Keep jsdoc/check-param-names

jsnajdr commented 2 years ago

For me personally, I only find useful the rules that say "I was able to detect there's something obviously wrong about your jsdoc comment", ones that enforce some consistency, like newline between description and params or {boolean} and {String} type names over {bool} and {string}.

On the other hand, rules that say "you need to fill in this required information" mostly lead to jsdoc comments that are repetitive, like:

/**
 * Function that converts dogs to cats
 *
 * @param {Dog} dog the dog
 * @returns {Cat} cat the cat
 */
function dogToCat( dog ) {
  /* ... */
  return cat;
}

where the jsdoc comment doesn't really say anything new.

This anti-pattern is very common in our Redux code where we are forced to document action creators as:

/**
 * @returns {Action} an action object to be dispatched
 */

or reducers as:

/**
 * @param state The original state
 * @param action An action object
 * @returns Updated state
 */

I would be better if the linter let the author decide what's worth documenting and let them omit the parts that are obvious.

noahtallen commented 2 years ago

I don't have much to add, but I agree with your logic and the proposal to change these rules.

If one wants great type analysis for a JS function, I think we should be encouraging them to re-write in TS rather than jsdoc. I guess it's even possible that jsdoc gives a false sense of security that types are checked by something before deploy.

IanVS commented 2 years ago

@scinos I'm excited to see such great use of eslint-nibble, and I hope it's been helpful. Let me know if you have any suggestions or requests for it.

Automattic / wp-calypso