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 UNC support to url.pathToFileURL() #34743

Closed
wants to merge 2 commits into from
Closed

lib: add UNC support to url.pathToFileURL() #34743

wants to merge 2 commits into from

Conversation

mceachen
Copy link
Contributor

Fixes: #34736

Checklist
  • make -j4 test (UNIX) passes
  • vcbuild test (Windows) passes
  • tests are included
  • commit message follows commit guidelines
  • certified Developer's Certificate of Origin 1.1

guybedford
guybedford approved these changes Aug 12, 2020
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.

This looks great to me, thank you so much @mceachen for the PR.

Let's get a review from @jasnell as well to land.

@mceachen
Copy link
Contributor Author

Seems like test-asan failed from something not related to this PR:

=== release test ===
Path: node-api/test_worker_terminate_finalization/test
##[error]--- stderr ---
=================================================================
==107572==ERROR: LeakSanitizer: detected memory leaks

Direct leak of 80 byte(s) in 1 object(s) allocated from:
    #0 0xa1d80d in operator new(unsigned long) (/home/runner/work/node/node/out/Release/node+0xa1d80d)
    #1 0xb5bf63 in napi_create_reference (/home/runner/work/node/node/out/Release/node+0xb5bf63)
    #2 0x7f5a275d1878  (<unknown module>)
    #3 0xb47492 in v8impl::(anonymous namespace)::FunctionCallbackWrapper::Invoke(v8::FunctionCallbackInfo<v8::Value> const&) (/home/runner/work/node/node/out/Release/node+0xb47492)
    #4 0x1268a73 in v8::internal::FunctionCallbackArguments::Call(v8::internal::CallHandlerInfo) (/home/runner/work/node/node/out/Release/node+0x1268a73)
    #5 0x12668a4 in v8::internal::MaybeHandle<v8::internal::Object> v8::internal::(anonymous namespace)::HandleApiCallHelper<false>(v8::internal::Isolate*, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::HeapObject>, v8::internal::Handle<v8::internal::FunctionTemplateInfo>, v8::internal::Handle<v8::internal::Object>, v8::internal::BuiltinArguments) (/home/runner/work/node/node/out/Release/node+0x12668a4)
    #6 0x1264932 in v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) (/home/runner/work/node/node/out/Release/node+0x1264932)

@rickyes rickyes added lib / src Issues and PRs related to general changes in the lib or src directory. request-ci Add this label to start a Jenkins CI on a PR. labels Aug 12, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 12, 2020
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Aug 12, 2020 •
edited by rickyes
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/32742/ (💚)

@jasnell
Copy link
Member

jasnell commented Aug 12, 2020

@mceachen ... yeah, that's a known issue. A fix is pending.

lpinca
lpinca reviewed Aug 12, 2020
View reviewed changes
lib/internal/url.js Outdated
function encodePathChars(filepath) {
if (filepath.includes('%'))
filepath = filepath.replace(percentRegEx, '%25');
// In posix, "/" is a valid character in paths
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// In posix, "/" is a valid character in paths
// In posix, "\" is a valid character in paths

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good catch. I just moved this comment, btw, when I refactored out the function. Will fix.

lib/internal/url.js Show resolved Hide resolved
test/parallel/test-url-pathtofileurl.js Show resolved Hide resolved
@mceachen
Update url.js
94a2126 
fix mistake in comment
@mceachen
Copy link
Contributor Author

mceachen commented Aug 13, 2020 •
edited
Loading

Thanks for the reviews! Is there anything else I need to do for this PR? (I haven't committed to this project before).

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Aug 13, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Aug 13, 2020
@nodejs-github-bot
Copy link
Collaborator

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

Trott pushed a commit that referenced this pull request Aug 14, 2020
@mceachen @Trott
lib: add UNC support to url.pathToFileURL()
7b8c6b0 
Fixes: #34736

PR-URL: #34743
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@Trott
Copy link
Member

Trott commented Aug 14, 2020

Landed in 7b8c6b0.

Thanks for the contribution! 🎉

@Trott Trott closed this Aug 14, 2020
MylesBorins pushed a commit that referenced this pull request Aug 17, 2020
@mceachen @MylesBorins
lib: add UNC support to url.pathToFileURL()
72e7380 
Fixes: #34736

PR-URL: #34743
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@danielleadams danielleadams mentioned this pull request Aug 20, 2020
Merged
BethGriggs pushed a commit that referenced this pull request Aug 20, 2020
@mceachen @BethGriggs
lib: add UNC support to url.pathToFileURL()
1a9496a 
Fixes: #34736

PR-URL: #34743
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@mceachen @addaleax
lib: add UNC support to url.pathToFileURL()
418ff2d 
Fixes: #34736

PR-URL: #34743
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
@mceachen @addaleax
lib: add UNC support to url.pathToFileURL()
f29ab40 
Fixes: #34736

PR-URL: #34743
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@guybedford guybedford guybedford approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

url.pathToFileURL doesn't generate valid URLs for UNC paths
7 participants
@mceachen @nodejs-github-bot @jasnell @Trott @guybedford @lpinca @rickyes