lib: fix JSDoc issues by Trott · Pull Request #45243 · nodejs/node

Trott · 2022-10-30T16:07:21Z

Updating ESLint and dependencies will start flagging a few additional
JSDoc issues. One or two of these are simple fixes. The ESM stuff
requires throwing explicitly in JSDoc'ed functions rather than calling
another function to throw. I think this makes the code easier to
understand--you don't need to know that a particular function that
starts with throwsIf might throw but something that starts with
throwsAnythingElse will always throw. Instead, it's right there in the
code. This also might make it easier to improve stack traces if that's
something we'd like to do at some point.

nodejs-github-bot · 2022-10-30T16:07:25Z

Review requested:

@nodejs/crypto
@nodejs/http
@nodejs/modules
@nodejs/net

Trott · 2022-10-30T16:07:32Z

For easier review, look at just the first commit.

nodejs-github-bot · 2022-10-31T03:43:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/47584/

JakobJingleheimer

I think this PR doesn't update all the functions/utilities needed to achieve its goal around "throwIf" might throw. For example, validation utilities throw on fail—a specific example that is kinda broken in this PR is validateAssertions():

node/lib/internal/modules/esm/assert.js

Line 82 in 5847043

throw new ERR_IMPORT_ASSERTION_TYPE_MISSING(url, validType);

node/lib/internal/modules/esm/assert.js

Line 84 in 5847043

return handleInvalidType(url, importAssertions.type);

It would now sometimes throw and sometimes return an error.

Prior to this PR, I think we have a fairly well established convention, which this breaks.

I'm not necessarily opposed to changing the convention, but if we do, we should fully change it (caveat though: there are heaps of validation utilities etc that would need updating).

lib/internal/modules/esm/assert.js

Trott · 2022-10-31T16:19:53Z

I think this PR doesn't update all the functions/utilities needed to achieve its goal around "throwIf" might throw.

That's correct. I only went as far as I needed to go to get JSDoc linting to stop complaining.

I'm not necessarily opposed to changing the convention, but if we do, we should fully change it (caveat though: there are heaps of validation utilities etc that would need updating).

OK, I'll try to make that happen with a subsequent commit to this branch. (Or if someone wants to do it before I get to it, please push the changes here!)

Updating ESLint and dependencies will start flagging a few additional JSDoc issues. One or two of these are simple fixes. The ESM stuff requires throwing explicitly in JSDoc'ed functions rather than calling another function to throw. I think this makes the code easier to understand--you don't need to know that a particular function that starts with `throwsIf` *might* throw but something that starts with `throwsAnythingElse` will always throw. Instead, it's right there in the code. This also might make it easier to improve stack traces if that's something we'd like to do at some point.

Trott · 2022-11-02T21:58:35Z

It would now sometimes throw and sometimes return an error.

validateAssertions() always throws still. The return is there to signal to the linter and to someone reading the code that you can't get any further in the function. Unfortunately, it also signals that a value is being returned, which...it isn't. Short of going through every single validation function and their call-sites and changing them to return errors rather than throw themselves, I'm not sure how to "fix" that or if it needs "fixing". Maybe disabling the lint rule for that function is the better option after all. (I personally prefer adding the return but I think this is very subjective.)

nodejs-github-bot · 2022-11-07T07:04:33Z

Landed in 73a5112...b13738a

Updating ESLint and dependencies will start flagging a few additional JSDoc issues. One or two of these are simple fixes. The ESM stuff requires throwing explicitly in JSDoc'ed functions rather than calling another function to throw. I think this makes the code easier to understand--you don't need to know that a particular function that starts with `throwsIf` *might* throw but something that starts with `throwsAnythingElse` will always throw. Instead, it's right there in the code. This also might make it easier to improve stack traces if that's something we'd like to do at some point. PR-URL: #45243 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>

PR-URL: #45243 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>

Updating ESLint and dependencies will start flagging a few additional JSDoc issues. One or two of these are simple fixes. The ESM stuff requires throwing explicitly in JSDoc'ed functions rather than calling another function to throw. I think this makes the code easier to understand--you don't need to know that a particular function that starts with `throwsIf` *might* throw but something that starts with `throwsAnythingElse` will always throw. Instead, it's right there in the code. This also might make it easier to improve stack traces if that's something we'd like to do at some point. PR-URL: nodejs#45243 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>

PR-URL: nodejs#45243 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>

Updating ESLint and dependencies will start flagging a few additional JSDoc issues. One or two of these are simple fixes. The ESM stuff requires throwing explicitly in JSDoc'ed functions rather than calling another function to throw. I think this makes the code easier to understand--you don't need to know that a particular function that starts with `throwsIf` *might* throw but something that starts with `throwsAnythingElse` will always throw. Instead, it's right there in the code. This also might make it easier to improve stack traces if that's something we'd like to do at some point. PR-URL: #45243 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>

PR-URL: #45243 Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Tobias Nießen <tniessen@tnie.de> Reviewed-By: Jan Krems <jan.krems@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Darshan Sen <raisinten@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Zeyu "Alex" Yang <himself65@outlook.com>

The nodejs#45243 upgraded eslint and apparently, when you specific a `@returns` early returns aren't considered valid. This PR fixes this lint issue.

The #45243 upgraded eslint and apparently, when you specific a `@returns` early returns aren't considered valid. This PR fixes this lint issue. PR-URL: #45409 Reviewed-By: Filip Skokan <panva.ip@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Ruy Adorno <ruyadorno@google.com> Reviewed-By: Beth Griggs <bethanyngriggs@gmail.com>