Increase severity of JSDoc to error.

[sainthkh]

• Closes eslint-plugin: Increase severity of JSDoc rules to error #18458

Description

• Updated eslint-plugin-jsdoc from 15.8.0 to 21.0.0.
• Increase severity of JSDoc to error.

How has this been tested?

• npm run lint-js command will not show any warnings.
• Env: Ubuntu 18.04

Notes about decisions

AS it is written in #18458, eslint-plugin-jsdoc cannot parse some TypeScript types like [key:string]: any. So, I fixed the issues like these:

• [key:string]: any: Mostly, Record<string, any>
• Added descriptions
• If all of above aren't possible, I used /* eslint-disable */, /* eslint-enable */ pair. Unfortunately, as JSDoc is written in comment, // eslint-disable-line doesn't work.

Screenshots

N/A

Types of changes

Minor JSDoc comment changes.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• [N/A] My code follows the accessibility standards.
• [N/A] My code has proper inline documentation.
• [N/A] I've included developer documentation if appropriate.
• [N/A] I've updated all React Native files affected by any refactorings/renamings in this PR.

[gziolo]

@dsifford and @aduth – what do you think about this proposal?

[dsifford]

I'll leave the decision to @aduth as he has more skin in the game than I do here.

[aduth]

I've not yet had time to review this in detail, but I'm of course happy to see these warnings bumped to errors.

What I'm not sure about on a cursory glance is whether we want to be changing the existing types to Record, since this is a lossy transition (see example, we lose the 'settings' name). In my mind, I had considered that we simply add inline ESLint configuration to temporarily disable the rules-checking, until such time that the upstream dependencies are updated to support the syntax (per #18458 (comment), tracked at jsdoctypeparser/jsdoctypeparser#50).

[sainthkh]

Rebased.

[aduth]

@sainthkh What do you think about my remarks at #20427 (comment) ? I'm not entirely comfortable with the changes to use Record.

[sainthkh]

I used Record because adding /* eslint-disable */ at the top and the bottom of JSDoc comment looks ugly and we don't know when this will be fixed.

After reading your comment and related code, I agree that restoring setting is better than removing it. setting is important here.

But I don't think key in {[key: string]: any} doesn't provide much information. In this case, I think Record is better. For the same reason, I prefer Record<WPKeycodeModifier, (key:string)=>any> to {[M in WPKeycodeModifier]:(key:string)=>any}.

These are my opinion. The downside is that it is not consistent.

If you think Record hurts consistency, I don't mind reverting them.

[sainthkh]

Rebased.

[aduth]

But I don't think key in {[key: string]: any} doesn't provide much information. In this case, I think Record is better. For the same reason, I prefer Record<WPKeycodeModifier, (key:string)=>any> to {[M in WPKeycodeModifier]:(key:string)=>any}.

I think it would depend. In this very specific case, you're right that key might not provide much value, because it's more or less a default term used when referring to the key of an object. But I don't think it would always be used this way.

Consider this example:

https://github.com/aduth/tannin/blob/cd1c7447843df7751c4abd1b92aee03fe56bfba4/packages/tannin/index.js#L38-L44

In that case, domain is a useful piece of information that would be lost by using Record.

That said, since we are talking about this specific case, I think it would be fine to use Record. We might want to consider whether it's a well-understood type (and as such, if we expect there to be any issue in developer onboarding).

This section of the current documentation is quite relevant, and might need to be updated accordingly:

https://github.com/WordPress/gutenberg/blob/master/docs/contributors/coding-guidelines.md#record-types

[aduth]

I'm trying to understand what the specific difference is between Object<string,string> and Record<string,string>.

Related: https://www.typescriptlang.org/docs/handbook/utility-types.html#recordkt

I have experienced in the past that TypeScript has a tendency to treat Object types as equivalent to the (dreaded) any type, which we most certainly would want to avoid. I guess Record is preferred in these cases where we want to loosely define the types of keys and values in what is expected to be provided as a plain object? I'm honestly not clear on this subject. If it is, that sounds quite nice actually.

[sainthkh]

In TypeScript, [Object<string, number>] doesn't exist. Check this playground. (If it doesn't show any code, type in let a: Object<string, number> = {}, you'll know it soon.)

And according to JSDoc doc, the correct way to specifying types is {Object.<string, number>}. Requires . in the middle.

One difference between Record and indexable interface is that you can use symbol type as a key in Record.

interface X {
    [x: symbol]: string // error
}

let a: Record<symbol, number> = {} // ok

But I guess it doesn't matter much. Because symbol isn't used a lot.

[sainthkh]

My proposal:

• If the object key name is meaningful like setting or domain, use indexable interface.
• If not, use Record.

[aduth]

That seems sensible to me. Would you want to include those documentation changes here as well?

[sainthkh]

Yes. I think that's the part of this update.

[sainthkh]

@aduth

• Opt out jsdoc/check-value.
• Restored setting in WPLinkControlSettingsValue.
• Added Record rule to the coding guideline.
• Wrote ESLint plugin: Relax the rule for @since in JSDoc #20859 for @SInCE.

Question:

• According to the guideline, it is incorrect to use {Object}, {Function}, {Promise}. But there is no rule to enforce it. Should we create one for this? If so, I'll create an issue for this, because there is none.
• If we decide to make a rule for them, it might be sensible to add an issue about removing them. In Gutenberg, there are 1304 {Object}s and 164 {Function}s and 34 {Promise}s.

[aduth]

Question:

• According to the guideline, it is incorrect to use {Object}, {Function}, {Promise}. But there is no rule to enforce it. Should we create one for this? If so, I'll create an issue for this, because there is none.
• If we decide to make a rule for them, it might be sensible to add an issue about removing them. In Gutenberg, there are 1304 {Object}s and 164 {Function}s and 34 {Promise}s.

These are good questions, especially considering that the document is pretty unambiguous in calling them "Incorrect" (to clarify, I'm the one who wrote it this way). At least speaking from my own experience, I've not always been successful in following these rules so strictly, and I don't know that we want to put many barriers in place which might dissuade people from using types. At the same time, we should want to encourage people as much as possible to use the best options available to them. Some of these are easier to adopt than others. Promise is pretty straight forward to provide with a generic type, and we should encourage it always. Function is sometimes a much more accessible option than the technically-better-but-harder-to-write expanded form. In some cases, such as in a @template tag like with higher-order functions, it might even be fair to say it can be the correct choice (example)?

So to specifically answer your question:

• Right now, I don't think we should want to enforce this.
• The documentation should perhaps be updated to encourage moreso than codify (phrasing like "Bad" vs. "Good" or "Not Ideal" vs. "Better", etc).
• We may still want to audit existing code to see where we can make updates. I'd be a bit hesitant that an issue would be the most effective way to track this, compared to simply setting a good precedent and clear documentation for people to make good judgment in their own refactoring.

I think any or all of these could change over time as we continue to explore how to implement types in the codebase. I'm also open to arguments to the contrary.

[aduth]

This is looking good!

One last thing I see is that the Coding Guidelines document still has quite a few references to Object types throughout. Should these be updated to Record, at least when used as a generic?

Related to my previous comment, there are also some interesting cases where it's not so clear what the best type would be. Notably, for React function components, how best should we document the props argument.

For example:

/**
 * Renders a toolbar.
 *
 * @param {Object} props             Component props.
 * @param {string} [props.className] Class to set on the container `<div />`.
 */

Is {Object} okay here? Is it something where we want to try to use the "indexable interface" format for describing the keys of the props object (I don't know that this is feasible since, unlike TypeScript, we're confined to describing the type in a single line)? I had wondered if the props @param line even provides any value, vs. just omitting and including only the specific properties. Last time I tried that, I think there were some errors related to the missing props documentation.

[sainthkh]

1.

In some cases, such as in a @template tag like with higher-order functions, it might even be fair to say it can be the correct choice (example)?

If we're using TypeScript, the type of F might be (...args: Array<any>) => any. It's a longer version of Function. We don't need to and can't use that in Gutenberg.

2.

Is {Object} okay here?

It seems that we have no other option for now.

/**
 *
 * @param {number} x xx
 * @param {number} y yy
 */
export const x = ( { x, y } ) => {
	return x + y;
};

When we write a simple function like above, TypeScript type checker will show us this error message:

packages/a11y/src/addContainer.js:44:20 - error TS8024: JSDoc '@param' tag has name 'y', but there is no parameter with that name.

44  * @param {number} y yy
                      ~

packages/a11y/src/addContainer.js:46:22 - error TS2339: Property 'x' does not exist on type 'Number'.

46 export const x = ( { x, y } ) => {
                        ~

packages/a11y/src/addContainer.js:46:25 - error TS2339: Property 'y' does not exist on type 'Number'.

46 export const x = ( { x, y } ) => {

3.

Is it something where we want to try to use the "indexable interface" format for describing the keys of the props object (I don't know that this is feasible since, unlike TypeScript, we're confined to describing the type in a single line)?

I think there's not much difference. And we all know that it's a React function by its name starting with a capital case. This is not a dictionary-like object that we want to add/remove elements on the run time. In addition, we don't need more /* eslint-disable jsdoc/check-type */.

4.

I checked the guideline and {Object} was used 4 times.

• Custom type definition -> It's OK there. It's not a dictionary.
• Generic {Object} or object as a dictionary -> It's changed to Record.
• Optional item in props object -> It's OK. Because it's not a dictionary.
• React component typing -> Same as above.

It seems that nothing should be changed.

5.

The best to way to enforce types is to migrate to TS and use strict option. But it's a big project and it's outside the topic of this project.

But like eslint warnings, when types aren't enforced, people ignore them. Maybe we need to discuss it later. (I know you had a meeting about TypeScript. And I also know that it can't be a priority now.)

[aduth]

When we write a simple function like above, TypeScript type checker will show us this error message:

In your example, I sense the individual properties would still need to include some object name in order to use the dot notation:

https://jsdoc.app/tags-param.html#parameters-with-properties

I was hoping it would work with just an arbitrary name (props, for a function component):

/**
 * Returns a person's name.
 *
 * @type {import('react').FC}
 *
 * @param {string} props.name Person name.
 */
function Person( { name } ) {
	return <div>{ name }</div>;
}

But the ESLint rule doesn't like it:

@param path declaration ("props.name") appears before any real parameter.eslint(jsdoc/check-param-names)

---

Custom type definition -> It's OK there. It's not a dictionary.

Are you referring to this one?

/**
 * A block selection object.
 *
 * @typedef {Object} WPBlockSelection
 *
 * @property {string} clientId     A block client ID.
 * @property {string} attributeKey A block attribute key.
 * @property {number} offset       An attribute value offset, based on the rich
 *                                 text value.
 */

If I recall correctly, we can remove {Object} altogether there, and it's understood to mean a type with those other properties. In fact, I feel I've run into issues where keeping {Object} can be problematic in how that type is used (I can't recall specifically what they were).

---

Optional item in props object -> It's OK. Because it's not a dictionary.

Which is this referring to?

---

It seems that nothing should be changed.

What about this one

 * @return {Object<string,WPDataSelector>} Object containing the store's
 *                                         selectors.

Should this be {Record<string,WPDataSelector>}, or no?

---

But like eslint warnings, when types aren't enforced, people ignore them. Maybe we need to discuss it later. (I know you had a meeting about TypeScript. And I also know that it can't be a priority now.)

There's definitely a desire to enforce types inferred from JSDoc. See #18838. It's already in place for a handful of modules. It's not quite to the same extent as going full TypeScript, but it is enforced where modules have been opted-in, and I'm highly in favor of more modules to be enabled.

---

To summarize, the only points above that I think need some decision or action:

• Can we remove {Object} from * @typedef {Object} WPBlockSelection in the documentation?
• Can we convert * @return {Object<string,WPDataSelector>} to {Record<string,WPDataSelector>} in the documentation?

[sainthkh]

I'm sorry. I searched {Object} and missed {Object<.

---

Can we remove {Object} from * @typedef {Object} WPBlockSelection in the documentation?

Yes. eslint and tsc don't complain about this. And @property lines below succinctly says that it's a type for an object (or interface in TypeScript jargon).

---

Can we convert * @return {Object<string,WPDataSelector>} to {Record<string,WPDataSelector>} in the documentation?

Yes. Object<T, K> doesn't exist. And it's a dictionary of items with special type. So, Record is right here.

---

Optional item in props object -> It's OK. Because it's not a dictionary.

Which is this referring to?

This one below:

/**
 * Renders a toolbar.
 *
 * @param {Object} props             Component props.
 * @param {string} [props.className] Class to set on the container `<div />`.
 */

I experimented if we can remove {Object} here like @typedef. But TypeScript wants it.

---

It seems that everything is OK. I'll go fix the coding guideline.

[aduth]

Thanks for the work here, and for your patience and well-reasoned thoughts during the many iterations. This looks good to land now 🎉