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

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.

Review requested:

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

For easier review, look at just the first commit.

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

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():

https://github.com/nodejs/node/blob/58470432cd7f944e4d7b231b3d987dbb1ecfd9c2/lib/internal/modules/esm/assert.js#L82
https://github.com/nodejs/node/blob/58470432cd7f944e4d7b231b3d987dbb1ecfd9c2/lib/internal/modules/esm/assert.js#L84

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).

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.

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.)

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: #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>

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 #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>

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 #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>

nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 30, 2022

Trott mentioned this pull request Oct 30, 2022

tools: update eslint to 8.26.0 #45239

Closed

MoLow approved these changes Oct 30, 2022

View reviewed changes

tniessen approved these changes Oct 30, 2022

View reviewed changes

hybrist approved these changes Oct 30, 2022

View reviewed changes

juanarbol approved these changes Oct 30, 2022

View reviewed changes

aduh95 approved these changes Oct 30, 2022

View reviewed changes

aduh95 added author ready PRs that have at least one approval, no outstanding review comments, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 30, 2022

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 30, 2022

This comment was marked as outdated.

Sign in to view

github-actions Bot mentioned this pull request Oct 31, 2022

CI Reliability 2022-10-31 nodejs/reliability#416

Open

19 tasks

RaisinTen approved these changes Oct 31, 2022

View reviewed changes

targos approved these changes Oct 31, 2022

View reviewed changes

JakobJingleheimer reviewed Oct 31, 2022

View reviewed changes

Comment thread lib/internal/modules/esm/assert.js Outdated

github-actions Bot mentioned this pull request Nov 1, 2022

CI Reliability 2022-11-01 nodejs/reliability#417

Open

17 tasks

lpinca approved these changes Nov 1, 2022

View reviewed changes

github-actions Bot mentioned this pull request Nov 2, 2022

CI Reliability 2022-11-02 nodejs/reliability#418

Open

15 tasks

Trott force-pushed the jsdoc-fixes branch 3 times, most recently from 1ec421e to a42be1a Compare November 2, 2022 21:18

Trott and others added 2 commits November 2, 2022 14:55

tools: update eslint to 8.26.0

d109e09

Trott force-pushed the jsdoc-fixes branch from a42be1a to d109e09 Compare November 2, 2022 21:56

nodejs-github-bot closed this Nov 7, 2022

Trott deleted the jsdoc-fixes branch November 7, 2022 15:22

This was referenced Nov 8, 2022

CI Reliability 2022-11-08 nodejs/reliability#422

Open

CI Reliability 2022-11-09 nodejs/reliability#423

Open

github-actions Bot mentioned this pull request Nov 10, 2022

CI Reliability 2022-11-10 nodejs/reliability#424

Open

20 tasks

RafaelGSS mentioned this pull request Nov 10, 2022

v19.1.0 proposal #45269

Merged

RafaelGSS added a commit to RafaelGSS/node that referenced this pull request Nov 10, 2022

lib: fix eslint early return

8e21b8f

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

RafaelGSS mentioned this pull request Nov 10, 2022

[v19.x] lib: fix eslint early return #45409

Closed

github-actions Bot mentioned this pull request Nov 11, 2022

CI Reliability 2022-11-11 nodejs/reliability#425

Open

19 tasks

danielleadams mentioned this pull request Dec 30, 2022

v18.13.0 release proposal #46025

Merged

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

Uh oh!

Uh oh!

lib: fix JSDoc issues - #45243

lib: fix JSDoc issues#45243
Trott wants to merge 2 commits into
nodejs:mainfrom
Trott:jsdoc-fixes

Trott commented Oct 30, 2022

Uh oh!

nodejs-github-bot commented Oct 30, 2022

Uh oh!

Trott commented Oct 30, 2022

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Oct 31, 2022

Uh oh!

JakobJingleheimer left a comment •
edited
Loading

Uh oh!

Uh oh!

Uh oh!

Trott commented Oct 31, 2022

Uh oh!

Trott commented Nov 2, 2022

Uh oh!

nodejs-github-bot commented Nov 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

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

Uh oh!

Uh oh!

lib: fix JSDoc issues - #45243

lib: fix JSDoc issues#45243Trott wants to merge 2 commits intonodejs:mainnodejs/node:mainfrom Trott:jsdoc-fixesTrott/io.js:jsdoc-fixesCopy head branch name to clipboard

Conversation

Trott commented Oct 30, 2022

Uh oh!

nodejs-github-bot commented Oct 30, 2022

Uh oh!

Trott commented Oct 30, 2022

Uh oh!

This comment was marked as outdated.

nodejs-github-bot commented Oct 31, 2022

Uh oh!

JakobJingleheimer left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Trott commented Oct 31, 2022

Uh oh!

Trott commented Nov 2, 2022

Uh oh!

nodejs-github-bot commented Nov 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

lib: fix JSDoc issues#45243
Trott wants to merge 2 commits into
nodejs:mainfrom
Trott:jsdoc-fixes

JakobJingleheimer left a comment •
edited
Loading