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

url: return backslashes from fileURLToPath on Windows #25349

Closed
wants to merge 1 commit into from
Closed

url: return backslashes from fileURLToPath on Windows #25349

wants to merge 1 commit into from

Conversation

zenparsing
Copy link

Fixes: #25265

Fixes url.fileURLToPath to return backslash-separated paths instead of forward-separated paths on Windows. This is my first PR to the repo so any and all feedback is welcome.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

Trott
Trott reviewed Jan 5, 2019
View reviewed changes
lib/internal/url.js
@@ -1289,6 +1291,7 @@ function getPathFromURLWin32(url) {
}
}
}
pathname = pathname.replace(forwardSlashRegEx, '\\');
Copy link
Member

Choose a reason for hiding this comment

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

If you think it makes anything more readable here or elsewhere, you can use path.win32.sep and/or path.posix.sep and/or path.sep. (No strong opinion from me on whether or not they should be used here. Just pointing 'em out in case you didn't know about 'em.)

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure I have a strong opinion either.

Trott
Trott reviewed Jan 5, 2019
View reviewed changes
test/parallel/test-url-fileurltopath.js Outdated
const url = require('url');

// Input must be string or URL
assert.throws(() => url.fileURLToPath(null));
Copy link
Member

Choose a reason for hiding this comment

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

Linter is complaining that there is no second argument in assert.throws() here and elsewhere. (You can run the JS linter locally with make lint-js or, I believe, vcbuild lint-js. Running make test or, I think, vcbuild test should run the linter as well.) So this instance might be:

assert.throws(() => { url.fileURLToPath(null); }, { code: 'ERR_INVALID_ARG_TYPE' });

We require the second argument because changing the code of the error is considered a breaking change. So we want the test to validate the code property at least.

Copy link
Author

Choose a reason for hiding this comment

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

I added the second args. Thanks!

@mscdex
Copy link
Contributor

mscdex commented Jan 5, 2019

FWIW there is already a PR for this change in #25324

@zenparsing
Copy link
Author

@mscdex Ah, I didn't the other PR. Feel free to use that one if the group think it's higher quality. (Although of course I think this one is better! 😉)

guybedford
guybedford approved these changes Jan 5, 2019
View reviewed changes
Copy link
Contributor

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Looks good, and nice to have lots of tests here definitely.

bzoz
bzoz approved these changes Jan 7, 2019
View reviewed changes
Copy link
Contributor

@bzoz bzoz left a comment

Choose a reason for hiding this comment

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

This has better tests than my #25324, I'm going to close that other PR.

@bzoz bzoz mentioned this pull request Jan 7, 2019
Closed
3 tasks
@bzoz
Copy link
Contributor

bzoz commented Jan 7, 2019

CI: https://ci.nodejs.org/job/node-test-pull-request/19964/

zenparsing
zenparsing commented Jan 7, 2019
View reviewed changes
test/parallel/test-fs-whatwg-url.js
@@ -63,7 +63,7 @@ if (common.isWindows) {
code: 'ERR_INVALID_ARG_VALUE',
type: TypeError,
message: 'The argument \'path\' must be a string or Uint8Array without ' +
'null bytes. Received \'c:/tmp/\\u0000test\''
'null bytes. Received \'c:\\\\tmp\\\\\\u0000test\''
Copy link
Author

Choose a reason for hiding this comment

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

Updated this test after I noticed the error.

The reason for multiple backslashes is that ERR_INVALID_ARG_VALUE calls inspect on the invalid argument value.

@bzoz
Copy link
Contributor

bzoz commented Jan 7, 2019

Resumed CI: https://ci.nodejs.org/job/node-test-pull-request/19965/

@zenparsing
Copy link
Author

Is the failure

Couldn't find remote ref refs/heads/jenkins-node-test-commit-windows-fanned-fb44f14a3589ef5560470b8cf17145fd2f469d8b-bin-win-vs2017

because I force-pushed my latest update? Or something else?

Thanks for your patience : )

@Trott
Copy link
Member

Trott commented Jan 7, 2019

because I force-pushed my latest update?

I don't know, but it happens sometimes. Here's a Rebuild: https://ci.nodejs.org/job/node-test-commit-windows-fanned/23794/

@addaleax
Copy link
Member

addaleax commented Jan 8, 2019

Let’s see; rebuild CI: https://ci.nodejs.org/job/node-test-pull-request/19996/

@Trott
Copy link
Member

Trott commented Jan 8, 2019

https://ci.nodejs.org/job/node-test-pull-request/19965/ passed except for Windows and https://ci.nodejs.org/job/node-test-commit-windows-fanned/23794/ passed for Windows, so I think we're good as far as CI goes. (No commits have been pushed or force-pushed since those runs.)

@jdalton jdalton added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 10, 2019
@danbev
Copy link
Contributor

danbev commented Jan 11, 2019

Landed in 7237eaa.

@danbev danbev closed this Jan 11, 2019
danbev pushed a commit that referenced this pull request Jan 11, 2019
@danbev
url: return backslashes from fileURLToPath on win
7237eaa 
PR-URL: #25349
Fixes: #25265
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
addaleax pushed a commit that referenced this pull request Jan 14, 2019
@addaleax
url: return backslashes from fileURLToPath on win
6e716ed 
PR-URL: #25349
Fixes: #25265
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
@BridgeAR BridgeAR mentioned this pull request Jan 16, 2019
Merged
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jan 16, 2019
@BridgeAR
url: return backslashes from fileURLToPath on win
613d2b3 
PR-URL: nodejs#25349
Fixes: nodejs#25265
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2019
Merged
BethGriggs pushed a commit that referenced this pull request Apr 28, 2019
@BethGriggs
url: return backslashes from fileURLToPath on win
b49889d 
PR-URL: #25349
Fixes: #25265
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
@BethGriggs BethGriggs mentioned this pull request May 1, 2019
Merged
BethGriggs pushed a commit that referenced this pull request May 10, 2019
@BethGriggs
url: return backslashes from fileURLToPath on win
c0accb8 
PR-URL: #25349
Fixes: #25265
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 16, 2019
@MylesBorins
url: return backslashes from fileURLToPath on win
3f51a60 
PR-URL: #25349
Fixes: #25265
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Bartosz Sosnowski <bartosz@janeasystems.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
This was referenced May 29, 2019
Closed
Closed
Closed
Closed
Closed
@GeoffreyBooth GeoffreyBooth mentioned this pull request Sep 6, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott left review comments

@bzoz bzoz bzoz approved these changes

@lpinca lpinca lpinca approved these changes

@guybedford guybedford guybedford approved these changes

@jdalton jdalton jdalton approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

fileURLToPath returns forward slashes on Windows
9 participants
@zenparsing @mscdex @bzoz @Trott @addaleax @danbev @jdalton @guybedford @lpinca