-
Notifications
You must be signed in to change notification settings - Fork 29.1k
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
path: add fromURL
#44315
path: add fromURL
#44315
Conversation
CC @nodejs/path |
400a742
to
7628fdd
Compare
Is my understanding correct that this API is essentially just |
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.
Can we add tests for when the passed argument is not a string nor a URL
instance?
correct. see #41521 (comment) |
7628fdd
to
430991e
Compare
430991e
to
e23b518
Compare
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.
Can you add a test case with the empty string?
e23b518
to
0841725
Compare
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.
I don't have a strong opinion on this PR; this already seems to be a one-liner in userland.
@@ -267,6 +267,38 @@ path.format({ | |||
// Returns: 'C:\\path\\dir\\file.txt' | |||
``` | |||
|
|||
## `path.fromURL(pathOrUrl)` |
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.
Isn't the name misleading, given that the input is not necessarily a URL
? In fact, URL strings of the form file:///home
are ignored and not treated as URLs.
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.
what would you consider to be a better name?
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.
What about url.toPathIfFileURL
?
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.
Or maybe path.fromURLInstance
?
// Returns: 'file:///Users/node/dev' | ||
|
||
path.fromURL(new URL('file:///c:/foo.txt')); | ||
// Returns On Windows: 'c:\\foo.txt' |
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.
I am still unsure if I like the fact that fromURL(URL)
normalizes the path in an OS-specific manner while fromURL(string)
does not. I would assume that
path.fromURL(url.pathToFileURL(absolutePath)) === path.fromURL(absolutePath)
but that does not seem to be true in all cases.
On the other hand, this seems to be what @sindresorhus expects.
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.
I agree this makes more sence:
path.fromURL('file:///Users/node/dev') === '/Users/node/dev'
and not 'file:///Users/node/dev'
@sindresorhus WDYT?
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.
How do you decide if the user refers to a file: URL or to a relative path inside a directory named file:
?
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.
I agree this makes more sence:
path.fromURL('file:///Users/node/dev') === '/Users/node/dev'
and not'file:///Users/node/dev'
I think that's orthogonal to my concern regarding OS-specific path normalization, and not what this comment is about.
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.
I am actually less concerned about that since you can be explicit and use path.win32
/path.posix
, like we do in our tests
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.
Again, that's also not what my comment is about. My concern is the fact that fromURL
appears to sometimes -- but not always -- normalize the path.
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.
@tniessen do you think we should change the behavior even if the name of the method is changed to one of the names suggested here (toPathIfFileURL
/fromURLInstance
)?
sep: '/', | ||
delimiter: ':', | ||
win32: null, | ||
posix: null | ||
}; | ||
|
||
|
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.
Nit: unrelated change.
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.
Just making sure #44315 (comment) is resolved before this can land.
Fixes: #41521