Skip to content
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: add typings for fileURLToPath #38182

Closed
wants to merge 3 commits into from
Closed

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Apr 10, 2021

This types internal/url.js fileURLToPath.

It actually adds a typings to 3 locations:

  • getPathFromURLWin32 to specify the parameter it takes
  • URL.prototype.pathname to make inference work on the return type of getPathFromURLWin32
  • fileURLToPath to specify the parameters it takes

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 :

"checkJs": false,

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:

  • before

Screen Shot 2021-04-09 at 20 47 59

*after
Screen Shot 2021-04-09 at 20 47 39

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 10, 2021
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it draft?

@targos
Copy link
Member

targos commented Apr 10, 2021

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.

@aduh95 aduh95 removed the needs-ci PRs that need a full CI run. label Apr 10, 2021
@aduh95
Copy link
Contributor

aduh95 commented Apr 10, 2021

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.

@bmeck
Copy link
Member Author

bmeck commented Apr 10, 2021

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

@bmeck
Copy link
Member Author

bmeck commented Apr 10, 2021

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

@@ -1365,6 +1372,10 @@ function getPathFromURLPosix(url) {
return decodeURIComponent(pathname);
}

/**
* @param {string | URL} path
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe these need to be wrapped in parentheses:

* @param {(string | URL)} path

jsdoc→types→type union

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/**
* @param {string | URL} path
* @returns {string}
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we document thrown errors using jsdoc's @throws tag?
This function throws TypeError in two different circumstances with varying error codes.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

targos pushed a commit that referenced this pull request Apr 24, 2021
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>
targos pushed a commit that referenced this pull request Apr 29, 2021
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>
targos pushed a commit that referenced this pull request May 30, 2021
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>
targos pushed a commit that referenced this pull request Jun 5, 2021
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>
targos pushed a commit that referenced this pull request Jun 11, 2021
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>
@bmeck bmeck closed this Oct 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants