-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
url: allow use of URL with http.request and https.request #10638
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -908,8 +908,32 @@ function domainToUnicode(domain) { | |
return binding.domainToUnicode(String(domain)); | ||
} | ||
|
||
// Utility function that converts a URL object into an ordinary | ||
// options object as expected by the http.request and https.request | ||
// APIs. | ||
function urlToOptions(url) { | ||
var options = { | ||
protocol: url.protocol, | ||
host: url.host, | ||
hostname: url.hostname, | ||
hash: url.hash, | ||
search: url.search, | ||
pathname: url.pathname, | ||
path: `${url.pathname}${url.search}`, | ||
href: url.href | ||
}; | ||
if (url.port !== '') { | ||
options.port = Number(url.port); | ||
} | ||
if (url.username || url.password) { | ||
options.auth = `${url.username}:${url.password}`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't support |
||
} | ||
return options; | ||
} | ||
|
||
exports.URL = URL; | ||
exports.originFor = originFor; | ||
exports.domainToASCII = domainToASCII; | ||
exports.domainToUnicode = domainToUnicode; | ||
exports.encodeAuth = encodeAuth; | ||
exports.urlToOptions = urlToOptions; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,11 @@ | ||
// Flags: --expose-internals | ||
'use strict'; | ||
|
||
require('../common'); | ||
|
||
const URL = require('url').URL; | ||
const assert = require('assert'); | ||
const urlToOptions = require('internal/url').urlToOptions; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe this one deserves a separate test? The name of this test doesn't really match this API here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not convinced that it matters too much either way, but there's no harm in separating it out. |
||
|
||
const url = new URL('http://user:[email protected]:21/aaa/zzz?l=24#test'); | ||
const oldParams = url.searchParams; // for test of [SameObject] | ||
|
@@ -125,3 +127,18 @@ assert.strictEqual(url.toString(), | |
'https://user2:[email protected]:23/aaa/bbb?k=99#abcd'); | ||
assert.strictEqual((delete url.searchParams), true); | ||
assert.strictEqual(url.searchParams, oldParams); | ||
|
||
// Test urlToOptions | ||
{ | ||
const opts = | ||
urlToOptions(new URL('http://user:[email protected]:21/aaa/zzz?l=24#test')); | ||
assert.strictEqual(opts instanceof URL, false); | ||
assert.strictEqual(opts.protocol, 'http:'); | ||
assert.strictEqual(opts.auth, 'user:pass'); | ||
assert.strictEqual(opts.hostname, 'foo.bar.com'); | ||
assert.strictEqual(opts.port, 21); | ||
assert.strictEqual(opts.path, '/aaa/zzz?l=24'); | ||
assert.strictEqual(opts.pathname, '/aaa/zzz'); | ||
assert.strictEqual(opts.search, '?l=24'); | ||
assert.strictEqual(opts.hash, '#test'); | ||
} |
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.
If I understand correctly this makes using the new URL api and using additional options(like
agent
) mutual exclusive?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.
True, but that's already the case if you pass the URL as a string or the result of
url.parse
. We can think later about a way to pass a URL object along with additional options but I don't think it should block this PR.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.
Ah I should have make this one a comment, not a change request :/ Sorry.
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.
No worries!
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.
Yes, making those mutually exclusive is intentional. Attaching additional non-standard properties to the URL object is not something that we should promote. And as @targos points out, that is already the case when passing the URL as a string.