Skip to content

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

Closed
mceachen wants to merge 2 commits into
nodejs:masterfrom
mceachen:master
Closed

lib: add UNC support to url.pathToFileURL()#34743
mceachen wants to merge 2 commits into
nodejs:masterfrom
mceachen:master

Conversation

@mceachen
Copy link
Copy Markdown
Contributor

@mceachen mceachen commented Aug 12, 2020

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
Copy Markdown
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.
@guybedford guybedford requested a review from jasnell August 12, 2020 02:02
@mceachen
Copy link
Copy Markdown
Contributor Author

mceachen commented Aug 12, 2020

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
Copy Markdown
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
Copy Markdown
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
Comment thread 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
Copy Markdown
Member

@lpinca lpinca Aug 12, 2020

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
Copy Markdown
Contributor Author

@mceachen mceachen Aug 12, 2020

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.
Comment thread lib/internal/url.js
Comment thread test/parallel/test-url-pathtofileurl.js
@mceachen
Update url.js
94a2126 
fix mistake in comment
@mceachen
Copy link
Copy Markdown
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
Copy Markdown
Collaborator

nodejs-github-bot commented Aug 14, 2020

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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@Trott
Copy link
Copy Markdown
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
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 <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@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

Labels

lib / src Issues and PRs related to general changes in the lib or src directory.

7 participants

@mceachen @nodejs-github-bot @jasnell @Trott @guybedford @lpinca @rickyes