lib: add typings for fileURLToPath - #38182
Conversation
Sorry, something went wrong.
targos
left a comment
There was a problem hiding this comment.
Why is it draft?
Sorry, something went wrong.
|
I feel like we should create a new subsystem for internal typings so that users who read our changelog don't interpret these commits the wrong way. |
Sorry, something went wrong.
|
I wonder if we could reuse the typings from https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/node instead of adding JSDoc comments now that we have a tsconfig.json. |
Sorry, something went wrong.
|
@aduh95 to some extent, however the public typings and the implementation typings likely serve different purposes for now at least. In particular implementation is about the actual checks for contributors which are not exactly the same as the public typings. I'd check each thing we pull over and see how we would want to pull in the LICENSE file if we do so. |
Sorry, something went wrong.
|
@targos it is a draft since people wanted to see an example PR for JSDoc and I'm not going to merge it for a bit. Also, I would love a new subsystem for this. |
Sorry, something went wrong.
| } | ||
|
|
||
| /** | ||
| * @param {string | URL} path |
There was a problem hiding this comment.
I believe these need to be wrapped in parentheses:
* @param {(string | URL)} path
Sorry, something went wrong.
There was a problem hiding this comment.
Core is targeting JSDoc support in TypeScript not jsdoc the documentation tool. We could add them, but they are not actually needed. Just be sure to stick with TS support and not break the practical usage of the tags is my thoughts. So don't use jsdoc.org but instead defer to TS
Sorry, something went wrong.
| /** | ||
| * @param {string | URL} path | ||
| * @returns {string} | ||
| */ |
There was a problem hiding this comment.
Should we document thrown errors using jsdoc's @throws tag?
This function throws TypeError in two different circumstances with varying error codes.
Sorry, something went wrong.
There was a problem hiding this comment.
from a practical perspective I don't think documenting the thrown errors is too useful since with current tooling it isn't actually able to guard them on a try/catch. In JS , stack depth, out of memory, etc. likely would cause such tags to be incorrect actually.
Sorry, something went wrong.
PR-URL: #38213 Refs: #38182 Refs: https://twitter.com/bradleymeck/status/1380643627211354115 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #38213 Refs: #38182 Refs: https://twitter.com/bradleymeck/status/1380643627211354115 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #38213 Refs: #38182 Refs: https://twitter.com/bradleymeck/status/1380643627211354115 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #38213 Refs: #38182 Refs: https://twitter.com/bradleymeck/status/1380643627211354115 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #38213 Refs: #38182 Refs: https://twitter.com/bradleymeck/status/1380643627211354115 Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: Michael Dawson <midawson@redhat.com> Reviewed-By: James M Snell <jasnell@gmail.com>
This types internal/url.js fileURLToPath.
It actually adds a typings to 3 locations:
I did not actually specify the return types while doing my initial check of these since that would be determined by the implementation details and specifying the return value might not match the implementation. Due to the desire to keep checks turned off by default due to the overall lack of typings at this point in the project. Be sure to enable checks by changing :
node/tsconfig.json
Line 10 in 40ace47
checkJs to true in node/tsconfig.json and being sure to restart any TS Server you might have running in your IDE to perform checks.
After enabling checks I set the expected return types and then validated that there were no errors before changing checkJs back to false and pushing out the PR.
There are no tests for such typings, but you can show proof of your work by attaching simple images which could then be validated by a reviewer:
*after
