url: allow use of URL with http.request and https.request #10638

Closed
wants to merge 2 commits into
from
@jasnell
Member
jasnell commented Jan 5, 2017

Allow the new URL object to be used with http.request() and https.request()

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

http, https

@jasnell jasnell added the semver-minor label Jan 5, 2017
@cjihrig
cjihrig approved these changes Jan 5, 2017 View changes
@targos
targos approved these changes Jan 5, 2017 View changes
lib/_http_client.js
+ pathname: options.pathname,
+ path: `${options.pathname}${options.search}`,
+ href: options.href
+ };
@addaleax
addaleax Jan 5, 2017 Member

I’m not sure how I feel about the code duplication here… could we make this a generic helper, possibly even hanging off the public url module (if that makes transitioning url.parse → url.URL easier)?

I would suggest adding it as the toJSON method for URL but I have looked at whatwg/url#137 and I’m not sure what to make of that other than that my idea would probably be silly/unrealistic/… :P

@addaleax
addaleax Jan 5, 2017 Member

Also – could this blocked be just executed independently of options instanceof url.URL? It would mean using path: options.path || …, but other than that, it should just work, no?

(If you feel unsure about these ideas, I can PR them myself later. If you feel they are horrible, say so and I won’t. ;))

@jasnell
jasnell Jan 5, 2017 Member

I can eliminate the duplication but having the instanceof and copying into a separate options is the most reliable and requires the fewest number of changes throughout the code.

@joyeecheung
joyeecheung Jan 6, 2017 edited Contributor

Those properties are enumerable (required by the spec) so a for-in loop with some ifs would do I think?

EDIT: util._extend doesn't apply because it only copies properties that are both enumerable and is own property. A helper function can loose the "isOwnProperty" bit but maybe not worth an additional abstraction anyway?

@jasnell
jasnell Jan 6, 2017 Member

I'd rather avoid doing the for-in loop in favor of keeping things more explicit and obvious. Also, as you point out, the auth vs username+password would demonstrates that the properties do not match up one-to-one.

lib/_http_client.js
+ } else if (options instanceof url.URL) {
+ options = {
+ protocol: options.protocol,
+ host: options.host,
@joyeecheung
joyeecheung Jan 6, 2017 edited Contributor

Maybe a auth: `${options.username}:${options.password}` here(and in http.js too)?

EDIT: don't know why this comment appeared in another line, deleted that.

@@ -24,6 +24,18 @@ function ClientRequest(options, cb) {
if (!options.hostname) {
throw new Error('Unable to determine the domain name');
}
+ } else if (options instanceof url.URL) {
@joyeecheung
joyeecheung Jan 6, 2017 Contributor

If I understand correctly this makes using the new URL api and using additional options(like agent) mutual exclusive?

@targos
targos Jan 6, 2017 Member

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.

@joyeecheung
joyeecheung Jan 6, 2017 Contributor

Ah I should have make this one a comment, not a change request :/ Sorry.

@targos
targos Jan 6, 2017 Member

No worries!

@jasnell
jasnell Jan 6, 2017 Member

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.

@jasnell jasnell Improvements
02b136d
@jasnell
Member
jasnell commented Jan 6, 2017

@addaleax @joyeecheung ... updated! PTAL

'use strict';
require('../common');
const URL = require('url').URL;
const assert = require('assert');
+const urlToOptions = require('internal/url').urlToOptions;
@joyeecheung
joyeecheung Jan 7, 2017 Contributor

Maybe this one deserves a separate test? The name of this test doesn't really match this API here.

@jasnell
jasnell Jan 7, 2017 Member

I'm not convinced that it matters too much either way, but there's no harm in separating it out.

@joyeecheung
Contributor

LGTM. The tests could be split in another PR anyway.

review was updated to a LGTM

@jasnell jasnell added a commit that referenced this pull request Jan 9, 2017
@jasnell jasnell url: allow use of URL with http.request and https.request
PR-URL: #10638
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
0f62ee6
@jasnell
Member
jasnell commented Jan 9, 2017

Landed in 0f62ee6

@jasnell jasnell closed this Jan 9, 2017
@italoacasas italoacasas added a commit to italoacasas/node that referenced this pull request Jan 18, 2017
@jasnell @italoacasas jasnell + italoacasas url: allow use of URL with http.request and https.request
PR-URL: nodejs#10638
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
d24bf76
@italoacasas italoacasas added a commit to italoacasas/node that referenced this pull request Jan 19, 2017
@jasnell @italoacasas jasnell + italoacasas url: allow use of URL with http.request and https.request
PR-URL: nodejs#10638
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
2ae4d88
@italoacasas italoacasas added a commit to italoacasas/node that referenced this pull request Jan 23, 2017
@jasnell @italoacasas jasnell + italoacasas url: allow use of URL with http.request and https.request
PR-URL: nodejs#10638
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
958319e
@targos targos added a commit that referenced this pull request Jan 28, 2017
@jasnell @targos jasnell + targos url: allow use of URL with http.request and https.request
PR-URL: #10638
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
aaab268
@italoacasas italoacasas referenced this pull request Jan 29, 2017
Merged

v7.5.0 proposal #11062

@italoacasas italoacasas added a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@jasnell @italoacasas jasnell + italoacasas url: allow use of URL with http.request and https.request
PR-URL: nodejs#10638
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
2f9fdc4
@italoacasas italoacasas added a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@jasnell @italoacasas jasnell + italoacasas url: allow use of URL with http.request and https.request
PR-URL: nodejs#10638
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Michal Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
9c30bf1
@evanlucas evanlucas added a commit that referenced this pull request Jan 31, 2017
@evanlucas evanlucas 2017-01-31, Version 7.5.0 (Current)
Notable changes:

* crypto:
  * ability to select cert store at runtime (Adam Majer) #8334
  * Use system CAs instead of using bundled ones (Adam Majer) #8334
* deps:
  * upgrade npm to 4.1.2 (Kat Marchán) #11020
  * upgrade openssl sources to 1.0.2k (Shigeki Ohtsu) #11021
* doc: add basic documentation for WHATWG URL API (James M Snell) #10620
* process: add NODE_NO_WARNINGS environment variable (cjihrig) #10842
* url: allow use of URL with http.request and https.request (James M Snell) #10638

PR-URL: #11062
42b4cee
@evanlucas evanlucas added a commit that referenced this pull request Jan 31, 2017
@evanlucas evanlucas 2017-01-31, Version 7.5.0 (Current)
Notable changes:

* crypto:
  * ability to select cert store at runtime (Adam Majer) #8334
  * Use system CAs instead of using bundled ones (Adam Majer) #8334
* deps:
  * upgrade npm to 4.1.2 (Kat Marchán) #11020
  * upgrade openssl sources to 1.0.2k (Shigeki Ohtsu) #11021
* doc: add basic documentation for WHATWG URL API (James M Snell) #10620
* process: add NODE_NO_WARNINGS environment variable (cjihrig) #10842
* url: allow use of URL with http.request and https.request (James M Snell) #10638

PR-URL: #11062
a34f1d6
@evanlucas evanlucas added a commit that referenced this pull request Feb 1, 2017
@evanlucas evanlucas 2017-01-31, Version 7.5.0 (Current)
Notable changes:

* crypto:
  * ability to select cert store at runtime (Adam Majer) #8334
  * Use system CAs instead of using bundled ones (Adam Majer) #8334
* deps:
  * upgrade npm to 4.1.2 (Kat Marchán) #11020
  * upgrade openssl sources to 1.0.2k (Shigeki Ohtsu) #11021
* doc: add basic documentation for WHATWG URL API (James M Snell) #10620
* process: add NODE_NO_WARNINGS environment variable (cjihrig) #10842
* url: allow use of URL with http.request and https.request (James M Snell) #10638

PR-URL: #11062
a1c91ec
@stevenvachon stevenvachon referenced this pull request in sindresorhus/got Feb 13, 2017
Open

WHATWG-URL support #228

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment