Skip to content

lib: add typings for fileURLToPath #38182

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

Closed
bmeck wants to merge 3 commits into from
Closed

lib: add typings for fileURLToPath #38182

Show file tree
Hide file tree
Changes from 1 commit
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
Prev Previous commit
Next Next commit
add returns checks
  • Loading branch information
@bmeck
bmeck committed Apr 10, 2021
commit 10f2010b986095c0ac1fd532e4d06564934e25a5
3 changes: 2 additions & 1 deletion lib/internal/url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1317,9 +1317,9 @@ const forwardSlashRegEx = /\//g;

/**
* @param {URL} url
* @returns {string}
*/
function getPathFromURLWin32(url) {
url.pathname
const hostname = url.hostname;
let pathname = url.pathname;
for (let n = 0; n < pathname.length; n++) {
Expand Down Expand Up @@ -1374,6 +1374,7 @@ function getPathFromURLPosix(url) {

/**
* @param {string | URL} path

@JakobJingleheimer JakobJingleheimer Apr 13, 2021

Copy link
Copy Markdown
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

@bmeck bmeck Apr 19, 2021

Copy link
Copy Markdown
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}
*/

@marsonya marsonya Apr 19, 2021

Copy link
Copy Markdown
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.

@bmeck bmeck Apr 19, 2021

Copy link
Copy Markdown
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')
Expand Down
2 changes: 1 addition & 1 deletion tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
],
"compilerOptions": {
"allowJs": true,
"checkJs": false,
"checkJs": true,
"noEmit": true,
"lib": ["ESNext"],
"target": "ESNext",
Expand Down