Add more JSDoc annotations + validate them and refactor to make them clearer

[voxpelli]

What is the purpose of this pull request? (put an "X" next to item)

[ ] Documentation update
[ ] Bug fix
[ ] New feature
[x] Other, please explain: To make #234 simpler to solve

What changes did you make? (Give an overview)

The main changes:

1. I extended the JSDoc types + added a validation of them using the TypeScript cli utility – this to better ensure that the public API contract doesn't get broken when I, @divlo or someone else new to the code contributes.
2. In the process of adding the types I found that parseOpts() was a bit all over the place and unlike its name was actually mainly focused on simply constructing the needed ESLint Config. I therefore revamped it to be fully focused on that + extracted it into a file of its own to make both it and the main file shorter and more focused.
3. As a result of the revamping of parseOpts() and adding types, the custom parseOpts argument added in Stop package.json settings from overriding explicit options #146 also had to get a modification to it (or be removed – I searched through the entirety of GitHub and there's no public usage there of it at least). I replaced it with a new resolveEslintConfig argument which mimics the original one.

Is there anything you'd like reviewers to focus on?

• Whether the general approach is good and where we want to take this
• Actually test this with standard itself

Why is this a draft PR?

I haven't actually pulled in this version into standard itself and tried it there yet, wanted to get this up for some more eyes first, as eg. @divlo has been working in this area as well.

[welcome]

🙌 Thanks for opening this pull request! You're awesome.

[voxpelli]

With these types it would also be possible to generate .d.ts files from this code, if we would like to publish with types.

Also – I failed to mention – the type strictness is pretty much to the max right now, because that's how I like it so that was what I aimed for. If others finds that annoyingly strict, then we can absolutely relax them – now or later.

[theoludwig]

Whether the general approach is good and where we want to take this

I also like to add types to my JavaScript, I don't write JavaScript anymore for my personal project, whenever I can, I use TypeScript.
With this PR, we are nearly like using TypeScript and in fact, we are installing it to check the JSDoc types.

I would argue that we should choose the right tool for the right job, so if we want to add more types, it's probably better to transition the entire project to TypeScript, that would make more sense in my opinion.

I would be 100% agree to do this, but that would be a major change for the project, and it is more a decision to take from the creator of this original codebase: @feross, @Flet, @LinusU.

Actually test this with standard itself

Yes, we should definitely test that standard/standard don't break with this major change. 👍

I searched through the entirety of GitHub and there's no public usage there of it at least

That still means, it is a BREAKING CHANGE, even if no one is actually using it.

---

Even if I don't agree with the current implementation (as the implementation should use TypeScript in my opinion), thank you for taking this PR, it will definitely make #234 simpler to solve, and the main idea behind this PR is great. 😄

[voxpelli]

Whether the general approach is good and where we want to take this

I also like to add types to my JavaScript, I don't write JavaScript anymore for my personal project, whenever I can, I use TypeScript.
With this PR, we are nearly like using TypeScript and in fact, we are installing it to check the JSDoc types.

I would argue that we should choose the right tool for the right job, so if we want to add more types, it's probably better to transition the entire project to TypeScript, that would make more sense in my opinion.

I would be 100% agree to do this, but that would be a major change for the project, and it is more a decision to take from the creator of this original codebase: @feross, @Flet, @LinusU.

We actually had JSDoc types before, but they were not validated and was hence quite incomplete. So basically what I mostly did was to add validation of those JSDoc comments, to help me complete them and to help us from avoiding regressions in them.

On TypeScript – it essentially consists of three parts – the validator, the type compiler and the code compiler – and one can use the validator and type compiler on JS code just as well as one can on TS code, but of course the code compiler is of no use for JS code as JS is already JS.

For a project of this size and complexity I would say that adding in a transpiler of any kind, TypeScript or otherwise, would be quite overkill and add nothing extra than what a JSDoc annotated JS gives 🙂 Except for a hard dependency on a transpiler step.

I searched through the entirety of GitHub and there's no public usage there of it at least

That still means, it is a BREAKING CHANGE, even if no one is actually using it.

Good point, I very much meant to write BREAKING CHANGE, it indeed is and since this is intended to target 15.x it feels ok.

I'm do am wondering though whether it makes most sense to migrate the API to a new one or whether just dropping it would be better, when we know of no current use cases.

[theoludwig]

Overall, it Looks Good To Me. 👍
But I would rather have a second review approval before merging this.
Also as you said, we should test this with standard itself.

We actually had JSDoc types before, but they were not validated and was hence quite incomplete. So basically what I mostly did was to add validation of those JSDoc comments, to help me complete them and to help us from avoiding regressions in them.

You actually changed my mind. 😄
You are not wrong there, we were already using JSDoc comments, so why not add some extra checks for the CI to avoid regressions in the future. 👍
But I still think a better implementation would be to migrate the project completely to TypeScript.

For a project of this size and complexity I would say that adding in a transpiler of any kind, TypeScript or otherwise, would be quite overkill and add nothing extra than what a JSDoc annotated JS gives slightly_smiling_face Except for a hard dependency on a transpiler step.

The real ask there is: when is the size of the project sufficient, to use TypeScript ?
If the project becomes bigger and bigger, it will be even harder to migrate the project to TypeScript, right?
So I would say, it is not right in my opinion and TypeScript is the right tool no matter the size of the project.

The main advantages to use TypeScipt is the better syntax to do the same thing :

/** type {string[]} */
const result = []

vs

const result: string[] = []

/**
 * @typedef StandardCliOptions
 * @property {import('../').linter} [linter]
 * @property {string} [cmd]
 * @property {string} [tagline]
 * @property {string} [homepage]
 * @property {string} [bugs]
 */

vs

import type { linter as Linter } from '../'

interface StandardCliOptions {
  linter: Linter
  cmd: string
  tagline: string
  homepage: string
  bugs: string
}

[LinusU]

Looks good 👍

Is this intended to go out as a major change, sine parseOpts was changed to resolveEslintConfig?

[voxpelli]

Is this intended to go out as a major change, sine parseOpts was changed to resolveEslintConfig?

Yes, I've started a milestone for a 15.0.0 release, thinking we do this + some of the other stuff mentioned in #234 + the #234 fix itself (I guess me, @divlo or someone else can extract other stuff into standalone issues – like the "move to Promises", "move to ES6 class")

[theoludwig]

(I guess me, @divlo or someone else can extract other stuff into standalone issues – like the "move to Promises", "move to ES6 class")

Yep, we better separate this in multiple issues/PRs, as it is quite a big change. 😄
New issues availables: #249, #250

Yes, I've started a milestone for a 15.0.0 release

@voxpelli This release also includes #232 (already been merged).
Do you think, we do #234, #249, #250 and we release a new version or we can also include #231 in v15 ?
I mean we could make standard@17 a big release.

[voxpelli]

Yes, I've started a milestone for a 15.0.0 release

@voxpelli This release also includes #232 (already been merged).
Do you think, we do #234, #249, #250 and we release a new version or we can also include #231 in v15 ?
I mean we could make standard@17 a big release.

Makes sense to add #231 in there as well, I added it to the milestone now 👍, had totally missed reading up on that one.

Slight wish: That we merge this PR first 😛

[theoludwig]

Maybe we can rename this test, to use resolveEslintConfig instead of parseOpts reference.
(Same thing for line 51).

[theoludwig]

Slight wish: That we merge this PR first 😛

Sure, it makes sense!
Is there is anything missing to merge this PR and to make it "Ready for review" instead of "Draft"? @voxpelli

Once this PR has been merged, I'm happy to work on these issues: #250 and #251
I also opened a new PR to drop Node.js v10 support: #252

Feel free to take #234 and #249, it would be awesome if you could tackle them! Thanks! 😄

Actually test this with standard itself

Be aware that there is already a test for that and it is already passing (see: test/clone.js).

[voxpelli]

Actually test this with standard itself

Be aware that there is already a test for that and it is already passing (see: test/clone.js).

Did not know! Let's merge then :)

[welcome]

🎉 Congrats on getting your first pull request landed!