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

fileURLToPath returns forward slashes on Windows #25265

Closed
zenparsing opened this issue Dec 29, 2018 · 6 comments
Closed

fileURLToPath returns forward slashes on Windows #25265

zenparsing opened this issue Dec 29, 2018 · 6 comments
Labels
confirmed-bug Issues with confirmed bugs. url Issues and PRs related to the legacy built-in url module. windows Issues and PRs related to the Windows platform.

Comments

@zenparsing
Copy link

  • Version: v11.6.0
  • Platform: Win64
  • Subsystem: url

url.fileURLToPath results in a path containing forward slashes instead of backslashes on Windows.

url.fileURLToPath('file:///C:/Users/zenparsing/Code');
// 'C:/Users/zenparsing/Code'
@devsnek
Copy link
Member

devsnek commented Dec 29, 2018

cc @nodejs/url

@vsemozhetbyt vsemozhetbyt added url Issues and PRs related to the legacy built-in url module. windows Issues and PRs related to the Windows platform. labels Dec 29, 2018
@seishun
Copy link
Contributor

seishun commented Dec 29, 2018

As far as I can tell, this function has never worked as documented, even in the commit that added it (eef072f):

> url.fileURLToPath('file:///C:/path/')
'C:/path/'

cc @guybedford

@zenparsing
Copy link
Author

It doesn't look like there are any tests for this function. I can put together a PR over the next couple of days, if that seems like a good direction.

@guybedford
Copy link
Contributor

@zenparsing thanks for reporting, that would be really great.

@guybedford
Copy link
Contributor

I guess the main thing is that there is the concept of whether / can be standardized as the method of windows paths support. See eg denoland/deno#957.

Personally I do prefer this style in cross-env app development, only doing \\ replacement when needed in arg parsing, but whether Node should have it by default is debatable.

@jdalton
Copy link
Member

jdalton commented Dec 31, 2018

It looks like this API was added without tests and this looks like a valid bug.
The docs for it show the expected result:

new URL('file:///C:/path/').pathname;    // Incorrect: /C:/path/
fileURLToPath('file:///C:/path/');       // Correct:   C:\path\ (Windows)

new URL('file://nas/foo.txt').pathname;  // Incorrect: /foo.txt
fileURLToPath('file://nas/foo.txt');     // Correct:   \\nas\foo.txt (Windows)

Currently the actual results on Windows are

url.fileURLToPath('file:///C:/path/') // Incorrect: 'C:/path/'
url.fileURLToPath('file://nas/foo.txt') // Incorrect: '//nas/foo.txt'

Other file path APIs (path.normalize, etc.) appear to return paths with backslashes on Windows.
Marking as a bug since it looks to meet the bar.

@jdalton jdalton added the confirmed-bug Issues with confirmed bugs. label Dec 31, 2018
bzoz added a commit to JaneaSystems/node that referenced this issue Jan 3, 2019
Makes fileURLToPath use backslashes as path separator on Windows

Fixes: nodejs#25265
zenparsing pushed a commit to zenparsing/node that referenced this issue Jan 7, 2019
@danbev danbev closed this as completed in 7237eaa Jan 11, 2019
addaleax pushed a commit that referenced this issue Jan 14, 2019
PR-URL: #25349
Fixes: #25265
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
BridgeAR pushed a commit to BridgeAR/node that referenced this issue Jan 16, 2019
PR-URL: nodejs#25349
Fixes: nodejs#25265
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
BethGriggs pushed a commit that referenced this issue Apr 28, 2019
PR-URL: #25349
Fixes: #25265
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
BethGriggs pushed a commit that referenced this issue May 10, 2019
PR-URL: #25349
Fixes: #25265
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
MylesBorins pushed a commit that referenced this issue May 16, 2019
PR-URL: #25349
Fixes: #25265
Reviewed-By: Guy Bedford <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Bartosz Sosnowski <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. url Issues and PRs related to the legacy built-in url module. windows Issues and PRs related to the Windows platform.
Projects
None yet
6 participants