-
Notifications
You must be signed in to change notification settings - Fork 29.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lib: improve hideStackFrames intellisense #44181
Conversation
Why do you say it was invalid? What warning do you see? |
In TypeScript, the return type for this kind of functions is |
@aduh95 The previous error for all functions that use
@targos Most of our validate functions does not return a value, but throw's an error if assertion fails. In that scenario, |
@anonrig Note the |
Can we not use the Closure Compiler syntax? We should be using JSDoc instead: /**
* @callback myCallback
* @param {number} mandatory
* @param {number} [optional]
*/
/** @type {myCallback} */
var cb; |
I did not really quite understand your point since JSDoc lacks certain functionality. JSDoc does not have /**
* @function validateOneOf
* @template T
* @param {T} value
* @param {string} name
* @param {T[]} oneOf
*/ |
Thanks for the review @aduh95. I've fixed them all. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, there's just one parameter that should be documented as optional
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the back and forth, I still have some questions/suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, LGTM!
Landed in ab9b590, thanks for the contribution 🎉 |
PR-URL: #44181 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs#44181 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: #44181 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: #44181 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: #44181 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: #44181 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: #44181 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs/node#44181 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
PR-URL: nodejs/node#44181 Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com>
This is a draft pull request to improve the developer experience by fixing internal function's JSDoc declarations. I wanted to receive some feedback before investing time on this.
Previously, all functions that uses hideStackFrames was invalidly typed and was shown as a warning in IDEs (On WebStorm and Sublime).
I wanted to introduce
T is boolean
kind of type guards into the functions but could not find a solution for functions that does not return any boolean, but throws an error.