Enable ESLint JSDoc checks

[colinrotherham]

I've enabled ESLint JSDoc checks in this PR

Links in with #2884

We do have quite a few issues flagged but also:

1. JSDoc missing entirely (is this alright?)
2. JSDoc wrongly flagging @jest-environment comments

Have a look at the automated checks either way

Update 1: I’ve enabled rules that cover (I think) the “style” we’d like to use and fixed errors in places. Warnings are still logged but we can fix those in our own time.

Update 2: All warnings now fixed

[colinrotherham]

@romaricpascal I've improved the config to:

1. Make JSDoc blocks optional or "description only"
2. Require a hyphen between param and description (matches TSDoc style)
3. Allow the unknown return type NodeListOf for .querySelectorAll()
4. Allow the unknown tag @jest-environment

All the warnings should be genuine ones now

[colinrotherham]

Why 'typescript' mode?

Rather than spend time documenting every complex object (or instance) we can use the declarations already published by the package author:

/**
 * @param {import('puppeteer').Page} page
 * @param {import('puppeteer').ElementHandle} element
 * @param {import('puppeteer').WaitForOptions} options
 */

TypeScript declarations are widely available unlike local-only @typedef tags:

[colinrotherham]

We sometimes hyphenate, sometimes don't, so I've added this rule

It's recommended by JSDoc (and mandatory for TSDoc)

If you provide a description, you can make the JSDoc comment more readable by inserting a hyphen before the description. Be sure to include a space before and after the hyphen.

[romaricpascal]

That's exciting 😄 Thanks for going through all the files as well, that must have been a bit tedious.

I added a few questions to help understand what's going on, only one of which may actually warrant some caution (the one about documenting the callback).

One thing I'm still unclear is where the line is regarding the safety we're looking to get from the JSDoc linting ahead of the release. Is it that the JSDoc we produce can be consumed as type definitions by VSCode?

[romaricpascal]

question: Why do we need to re-extend the 'plugin:jsdoc/recommended'? It's already declared in the root extends option.

[colinrotherham]

Well spotted! I thought I'd moved it, as it felt better in an override versus the root

Here I've made sure I'm only loading the JSDoc plugin for '**/*.{cjs,js,mjs}'

Gives us more flexibility when adding new plugins for other files. For example, if we added *.ts files in future their own override section would add the TSDoc plugin eslint-plugin-tsdoc instead

[romaricpascal]

question: Does that mean that a syntax like NodeList<Element> wouldn't be valid to highlight that our NodeList only contains elements?

[colinrotherham]

@romaricpascal Yeah exactly

Without this change then NodeList<Element> NodeListOf<Element> would be invalid

One of the JSDoc plugin dependencies must not be aware of it as a known DOM type (possibly because the TypeScript flavour is NodeListOf not NodeList. Haven't managed to track down exactly where to log this yet.

In Visual Studio Code if you mouse over .querySelectorAll() you'll see NodeListOf<Element>

[romaricpascal]

issue: We lost the separation of the sections within the list

suggestion: Would JSDoc be happy with the spacing of a Markdown list and leave the leading spaces alone ?

/*
 * - Arabic: Arabic (ar)
 * - Chinese: Burmese (my), Chinese (zh), Indonesian (id), Japanese (ja),
 *   Javanese (jv), Korean (ko), Malay (ms), Thai (th), Vietnamese (vi)
 * - French: Armenian (hy), Bangla (bn), French (fr), Gujarati (gu), Hindi (hi),
 *   Persian Farsi (fa), Punjabi (pa), Zulu (zu)
 * - German: Afrikaans (af), Albanian (sq), Azerbaijani (az), Basque (eu),
 *   Bulgarian (bg), Catalan (ca), Danish (da), Dutch (nl), English (en),
 *   Estonian (et), Finnish (fi), Georgian (ka), German (de), Greek (el),
 *   Hungarian (hu), Luxembourgish (lb), Norwegian (no), Somali (so),
 *   Swahili (sw), Swedish (sv), Tamil (ta), Telugu (te), Turkish (tr),
 *   Urdu (ur)
 * - Irish: Irish Gaelic (ga)
 * - Russian: Russian (ru), Ukrainian (uk)
 * - Scottish: Scottish Gaelic (gd)
 * - Spanish: European Portuguese (pt-PT), Italian (it), Spanish (es)
 * - Welsh: Welsh (cy)
 */

[colinrotherham]

@romaricpascal I'll have a look. Wonder if we need to specify markdown with extra formatting, like triple backticks?

[colinrotherham]

I've restored the previous lines and everything seems fine now

I must have run eslint --fix with some stricter settings earlier 🤦‍♂️

[romaricpascal]

question: Will that not catch all JavaScript files? Or is that necessary because we need to create an override section.

suggestion: We may want to exclude test files (in case we need file-specific helper functions sometimes)

Suggested change

	files: ['**/*.{cjs,js,mjs}'],
	files: ['**/*.{cjs,js,mjs}'],
	excludedFiles: ['*.test.js'],

[colinrotherham]

Yeah I'm happy with JSDoc checking all JavaScript files 😮

[romaricpascal]

question (there's one at the end, promise, it needs a bit of context 😆 )

I tried to run JSDoc on the project with npx jsdoc -c jsdoc.config.json src/**/*.{mjs,js} and the following config (to have it pick up mjs files):

{
  "source": {
    "includePattern": ".+\\.(js(doc|x)?|mjs)$",
    "excludePattern": ".+\\.test.(m|c)?js"
  },
  "plugins": ["plugins/markdown"]
}

The function definition as a type cannot be parsed by JSDoc, which complains about finding a space inside that bit 😬 I think it expects us to define the shape of the callback with @callback:

/**
 * @callback nodeListIterator
 * @param {Node} value - The current node being iterated on
 * @param {number} index - The current index in the iteration 
 * @param {NodeList} nodes - The NodeList itself
 * @returns {void}
 */

If we do that, though, VSCode won't be super helpful when you're using the function and just list "callback: nodeListIterator".

Are you aware of another that'd satisfy nice type and possible doc output? This may not be one for that PR, though, depending on what we're trying to get from the linting and JSDoc.

[colinrotherham]

Where's this JSDoc block from @romaricpascal?

[colinrotherham]

Ahh I see, that's your suggestion

Because we've set mode: 'typescript' for compatibility with node_modules that export type declarations, maybe for the jsdoc CLI you'll also need to enable the TypeScript plugin?

But it's simple enough to use @callback so pushed another commit up 👍

[colinrotherham]

Forgot to update this comment

I've added JSDoc generation via #2920 to confirm it all works

Type declarations like import('Puppeteer').Page are handled by the jsdoc-tsimport-plugin plugin

[36degrees]

I can't quite follow how this @param definition maps to the callback function as it is in the function – where do value, key and parent come from?

Would it make sense to use a @callback annotation here?

[colinrotherham]

Ahh sorry @36degrees that was @romaricpascal's suggestion too, it's a @callback now 🙌

To answer your question though I grabbed the names from Microsoft's forEach definition:

https://github.com/microsoft/TypeScript/blob/main/lib/lib.dom.d.ts#L10079

interface NodeListOf<TNode extends Node> extends NodeList {
    item(index: number): TNode;
    /**
     * Performs the specified action for each node in an list.
     * @param callbackfn  A function that accepts up to three arguments. forEach calls the callbackfn function one time for each element in the list.
     * @param thisArg  An object to which the this keyword can refer in the callbackfn function. If thisArg is omitted, undefined is used as the this value.
     */
    forEach(callbackfn: (value: TNode, key: number, parent: NodeListOf<TNode>) => void, thisArg?: any): void;
    [index: number]: TNode;
}

[romaricpascal]

question: Technically, the function doesn't really care about the NodeList containing Element which may. Do we need the extra typing to help completion when using the function?

[colinrotherham]

That's just me checking where it's called and could only see NodeListOf<Element>

Suppose you could have document.querySelector('#example').childNodes as NodeListOf<ChildNode>, shall I put both in the JSDoc? It's to avoid linting errors in TypeScript projects complaining that types are incompatible

Suggested change

	* @param {NodeListOf<Element>} nodes - NodeList from querySelectorAll()
	* @param {NodeListOf<Element | ChildNode>} nodes - NodeList from querySelectorAll()

[colinrotherham]

One thing I'm still unclear is where the line is regarding the safety we're looking to get from the JSDoc linting ahead of the release. Is it that the JSDoc we produce can be consumed as type definitions by VSCode?

Thanks @romaricpascal

In future, maybe, but not looking to generate type definitions yet.

We still need to remember that types might be automatically inferred from the JSDoc so we'll want to make sure they're all correct—eventually

I added some information on this issue: #2835 (comment)

[romaricpascal]

Having the rules set to warn rather than error would allow us to merge that configuration quickly, however I'm worried we won't have much of a gain from it. The test logs will just get filled with the current breakages and I'm not sure who will go there to fix them.

Having them as errors would force us to either not output JSDoc, or when we do it, do it to a consistent standard (even if we set the bar to a lower level than the plugin's recommended settings). It'd make more sense to me that this PR introducing linting does it to raise errors rather than risk the errors just laying around in the background, increasing the complexity of our linting for not much of a gain in the quality of our docs: we will only get highlights of issues in our editors, but no proper nudge to fix them (the editors should autofix a few things on save, though).

I would welcome raising errors and I'm happy to go around and fix the issues so we can start on a clean codebase. I'm not strongly attached to it, though and if people would prefer a softer introduction of this linting, then we could keep it as warnings. I'll let other @alphagov/design-system-developers voice their thoughts 😄