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.

Sign up for GitHub

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

Jump to bottom

lib: add typings for fileURLToPath #38182

Closed
wants to merge 3 commits into from
Closed

lib: add typings for fileURLToPath #38182

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91b412a
lib: add typings for fileURLToPath
bmeck Apr 10, 2021
10f2010
add returns checks
bmeck Apr 10, 2021
0ae4a4c
set checkJs back to false
bmeck Apr 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions lib/internal/url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -857,6 +857,9 @@ class URL {
FunctionPrototypeBind(onParsePortComplete, this));
}

/**
* @returns {string}
*/
get pathname() {
const ctx = this[context];
if (this[cannotBeBase])
Expand Down Expand Up @@ -1312,6 +1315,10 @@ function urlToHttpOptions(url) {

const forwardSlashRegEx = /\//g;

/**
* @param {URL} url
* @returns {string}
*/
function getPathFromURLWin32(url) {
const hostname = url.hostname;
let pathname = url.pathname;
Expand Down Expand Up @@ -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

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

function fileURLToPath(path) {
if (typeof path === 'string')
path = new URL(path);
Expand Down