http: allow url and options to be passed to http.request #21616

rubys · 2018-07-01T15:02:22Z

fixes #20795

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

mscdex · 2018-07-01T15:18:03Z

doc/api/http.md

 <!-- YAML
 added: v0.3.6
 changes:
+  - version: v11.0.0


version fields for additions/removals/changes should always have a value of REPLACEME

mscdex · 2018-07-01T15:19:00Z

doc/api/http.md

@@ -1920,10 +1924,13 @@ changes:
 Node.js maintains several connections per server to make HTTP requests.
 This function allows one to transparently issue requests.

-`options` can be an object, a string, or a [`URL`][] object. If `options` is a
+`url` can be an a string  or a [`URL`][] object. If `url` is a


There should only be one space between 'string' and 'or' here.

be an a -> be a

mscdex · 2018-07-01T15:19:30Z

doc/api/http.md

 string, it is automatically parsed with [`new URL()`][]. If it is a [`URL`][]
 object, it will be automatically converted to an ordinary `options` object.

+if both `url` and `options` are specified, the objects are merged, with the


mscdex · 2018-07-01T15:20:05Z

doc/api/http.md

 string, it is automatically parsed with [`new URL()`][]. If it is a [`URL`][]
 object, it will be automatically converted to an ordinary `options` object.

+if both `url` and `options` are specified, the objects are merged, with the
+options properties taking precedence.


s/options/`options`/

rubys · 2018-07-01T15:21:34Z

Notes:

went with a 3-arg signature with options overriding path if there is overlap (based on comment by @joyeecheung .
should be 100% backward compatible as URL is optional.
not sure how to test that the callback was identified correctly other than to run some requests and ensure that the callback is called. Not sure it is worth it?
HTTP/2 function signatures are enough different that this does not apply there.
HTTPS changes are doc only

vsemozhetbyt · 2018-07-01T15:22:42Z

Doc section headings affect autogenerated sections ids for section links. So each time we change a heading, we need to grep for the old hash and update all found instances. Currently, it seems these instances need to be updated (with #http_http_request_url_options_callback):

https://github.com/nodejs/node/blame/3d04e6c9a5522b6391a4a914ca16f2337abb35e7/doc/api/deprecations.md#L1042

https://github.com/nodejs/node/blame/8ab7ea6eed76d069dfd82684e2157e7d88badebf/doc/api/http.md#L2068

https://github.com/nodejs/node/blame/67790962daccb5ff19c977119d7231cbe175c206/doc/api/https.md#L365

vsemozhetbyt · 2018-07-01T15:26:33Z

Also these for https change (with #https_https_url_request_options_callback):

https://github.com/nodejs/node/blame/3d04e6c9a5522b6391a4a914ca16f2337abb35e7/doc/api/deprecations.md#L1044

https://github.com/nodejs/node/blame/67790962daccb5ff19c977119d7231cbe175c206/doc/api/https.md#L367

mscdex · 2018-07-01T15:28:26Z

doc/api/http.md

@@ -1869,16 +1869,20 @@ added: v0.5.9
 Global instance of `Agent` which is used as the default for all HTTP client
 requests.

-## http.request(options[, callback])
+## http.request([url] [, options] [, callback])


I feel like this should be something like: input[, options][, callback] to show that at least one argument is required and not that both are optional with perhaps additional explanation below or follow in the footsteps of documented functions like this and show multiple signatures for the same function. I'm not sure if we have a standard yet for showing multiple signatures for more complicated situations.

Ditto for https.request.

I followed the example of response.end.

I don't think it is a good idea to describe the syntax in a way that implies that url is required even if subsequent text says to ignore that implication.

Technically, nothing is required, as http.request() would be evaluated the same as http.request({ }).

Also, http.request accepts a url parameter despite ClientRequest giving this argument a different name so as to not collide with the url package which was previously required. Chose the name input based on Request().

FWIW, we have a section with various signatures:

https://nodejs.org/docs/latest/api/console.html#console_new_console_stdout_stderr_ignoreerrors
https://nodejs.org/docs/latest/api/console.html#console_new_console_options

mscdex · 2018-07-01T15:28:53Z

doc/api/https.md

 <!-- YAML
 added: v0.3.6
 changes:
+  - version: v11.0.0


Ditto about REPLACEME

mscdex · 2018-07-01T15:32:14Z

test/parallel/test-http-request-arguments.js

+const http = require('http');
+
+// two arguments: URL + options
+const request = http.request('http://example.com', { path: '/path' });


I think it would be better to have a real test by setting up a local http.Server on a random port, connecting to that, and verifying the path and such inside the incoming connection handler (wrapped in a common.mustCall()).

vsemozhetbyt · 2018-07-01T15:29:14Z

doc/api/http.md

 <!-- YAML
 added: v0.3.6
 changes:
+  - version: v11.0.0
+    pr-url: https://github.com/nodejs/node/pull/21616
+    description: allow url and options to be passed to http.request


Allow URL and options to be passed to `http.request()`.?

vsemozhetbyt · 2018-07-01T15:30:51Z

doc/api/http.md

@@ -1920,10 +1924,13 @@ changes:
 Node.js maintains several connections per server to make HTTP requests.
 This function allows one to transparently issue requests.

-`options` can be an object, a string, or a [`URL`][] object. If `options` is a
+`url` can be an a string  or a [`URL`][] object. If `url` is a


be an a -> be a

vsemozhetbyt · 2018-07-01T15:34:21Z

doc/api/https.md

 <!-- YAML
 added: v0.3.6
 changes:
+  - version: v11.0.0
+    pr-url: https://github.com/nodejs/node/pull/21616
+    description: allow url and options to be passed to http.request


Allow URL and options to be passed to `https.request()`.?

rubys · 2018-07-01T22:59:29Z

If we go with providing multiple signatures in the documentation, there is no need to update the autogenerated sections ids for section links. I still have a mild preference and believe that there is precedence for a function with all optional arguments, but I sense that I'm in the minority with that opinion.

Also, I noted that http.get and https.get have the same signature as their corresponding .request methods, so I updated them too to handle a separate url argument. The test case provided makes use of http.get(), which calls http.request(), which calls ClientRequest() .

Trott · 2018-07-01T23:04:39Z

@nodejs/http

vsemozhetbyt · 2018-07-01T23:19:56Z

doc/api/https.md

 <!-- YAML
 added: v0.3.6
 changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/21616
+    description: allow both url and options to be passed to `http.get()`


http -> https

vsemozhetbyt · 2018-07-01T23:22:29Z

doc/api/https.md

@@ -154,18 +159,22 @@ added: v0.5.9

 Global instance of [`https.Agent`][] for all HTTPS client requests.

-## https.request(options[, callback])
+## https.request([[url [,options] [, callback])


Should there also be 2 headings?

vsemozhetbyt · 2018-07-01T23:24:06Z

doc/api/http.md

@@ -1798,15 +1798,20 @@ The `requestListener` is a function which is automatically
 added to the [`'request'`][] event.

 ## http.get(options[, callback])
+## http.get(url [, options] [, callback])


Other signatures usually use fewer spaces, like (url[, options][, callback]).

mscdex · 2018-07-02T06:15:16Z

test/parallel/test-http-request-arguments.js

+        { hostname: 'localhost', port: server.address().port },
+        common.mustCall((res) => {
+          res.on('close', common.mustCall(() => {
+            server.close();


I think this could be moved to the server's request handler.

TimothyGu · 2018-07-03T21:11:32Z

lib/_http_client.js

  } else {
-    options = util._extend({}, options);
+    if (cb) throw new ERR_INVALID_ARG_TYPE('url', ['string', 'URL'], input);


I think this would make this PR a semver-major change, and I'd like to avoid that if possible. How about we just remove this check?

fixes nodejs#20795

rubys · 2018-07-26T20:19:35Z

I started to look at the backport (rubys@cbe9d7d), and found that the test/parallel/test-http-invalid-urls.js test was failing. While looking into that failure, I noticed that the https support is incomplete (there should have been a small change to lib/https.js, but it isn't there). I'll look into completing this change (with a new pull request, and ensuring that there is a test for https), and then circle back to backport both to 10.x.

Fixes: nodejs#20795 PR-URL: nodejs#21616 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Fixes: #20795 PR-URL: #21616 Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Backport-PR-URL: #21880 Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Notable changes: * deps: Upgrade V8 from 6.7 to 6.8 (Michaël Zasso) #21079 * Memory reduction and performance improvements, details at: https://v8project.blogspot.com/2018/06/v8-release-68.html * fs: Implement a fs.mkdir() recursive option, similar to the mkdirp npm package or mkdir -p on the command line (Benjamin Coe) #21875 * http: http.get() and http.request() (and https variants) can now accept three arguments to allow for a URL and an options object (Sam Ruby) #21616

Notable changes: * deps: Upgrade V8 from 6.7 to 6.8 (Michaël Zasso) #21079 * Memory reduction and performance improvements, details at: https://v8project.blogspot.com/2018/06/v8-release-68.html * fs: Implement a fs.mkdir() recursive option, similar to the mkdirp npm package or mkdir -p on the command line (Benjamin Coe) #21875 * http: http.get() and http.request() (and https variants) can now accept three arguments to allow for a URL and an options object (Sam Ruby) #21616 * Added new collaborators * Sam Ruby (https://github.com/rubys) * George Adams (https://github.com/gdams)

Notable changes: * buffer: * Fix out-of-bounds (OOB) write in `Buffer.write()` for UCS-2 encoding (CVE-2018-12115) * Fix unintentional exposure of uninitialized memory in `Buffer.alloc()` (CVE-2018-7166) * deps: * Upgrade to OpenSSL 1.1.0i, fixing: - Client DoS due to large DH parameter (CVE-2018-0732) - ECDSA key extraction via local side-channel (CVE not assigned) * Upgrade V8 from 6.7 to 6.8 (Michaël Zasso) #21079 - Memory reduction and performance improvements, details at: https://v8project.blogspot.com/2018/06/v8-release-68.html * http: `http.get()` and `http.request()` (and `https` variants) can now accept three arguments to allow for a `URL` _and_ an `options` object (Sam Ruby) #21616 * Added new collaborators * Sam Ruby (https://github.com/rubys) * George Adams (https://github.com/gdams)

Refs: nodejs#21616

PR-URL: #22949 Refs: #21616 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Jul 1, 2018

rubys force-pushed the issue-20795 branch 2 times, most recently from 8674cbb to 211140a Compare July 1, 2018 15:11

mscdex reviewed Jul 1, 2018

View reviewed changes

rubys force-pushed the issue-20795 branch from 211140a to 8ba9168 Compare July 1, 2018 15:19

mscdex reviewed Jul 1, 2018

View reviewed changes

rubys force-pushed the issue-20795 branch from 8ba9168 to cbd4166 Compare July 1, 2018 15:21

rubys force-pushed the issue-20795 branch from cbd4166 to 7100329 Compare July 1, 2018 15:23

mscdex reviewed Jul 1, 2018

View reviewed changes

doc/api/https.md Outdated

<!-- YAML

added: v0.3.6

changes:

- version: v11.0.0

Copy link

Contributor

mscdex Jul 1, 2018

Choose a reason for hiding this comment

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

Ditto about REPLACEME

mscdex reviewed Jul 1, 2018

View reviewed changes

vsemozhetbyt reviewed Jul 1, 2018

View reviewed changes

vsemozhetbyt added the https Issues or PRs related to the https subsystem. label Jul 1, 2018

rubys force-pushed the issue-20795 branch 2 times, most recently from ff0ed95 to ea111df Compare July 1, 2018 22:54

rubys force-pushed the issue-20795 branch from ea111df to 25c4b21 Compare July 1, 2018 23:02

vsemozhetbyt reviewed Jul 1, 2018

View reviewed changes

rubys force-pushed the issue-20795 branch from 25c4b21 to 7aef585 Compare July 2, 2018 01:07

mscdex reviewed Jul 2, 2018

View reviewed changes

rubys force-pushed the issue-20795 branch from 7aef585 to 55c2723 Compare July 2, 2018 10:48

TimothyGu reviewed Jul 3, 2018

View reviewed changes

http: allow url and options to be passed to http*.request and http*.get

4365379

fixes nodejs#20795

rubys force-pushed the issue-20795 branch from 55c2723 to 4365379 Compare July 3, 2018 21:28

rubys mentioned this pull request Jul 27, 2018

https: allow url and options to be passed to https.request #22003

Closed

3 tasks

targos added backported-to-v10.x and removed backport-requested-v10.x labels Aug 7, 2018

rvagg mentioned this pull request Aug 13, 2018

Release proposal: v10.9.0 #22295

Merged

targos mentioned this pull request Sep 19, 2018

how to do feature detection #22585

Closed

tniessen added a commit to tniessen/node that referenced this pull request Sep 19, 2018

doc: improve metadata for http.request

278ba4b

Refs: nodejs#21616

tniessen mentioned this pull request Sep 19, 2018

doc: add missing metadata #22949

Closed

3 tasks

refack mentioned this pull request Oct 18, 2018

build: spawn make test-ci with -j1 #23733

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

http: allow url and options to be passed to http.request #21616

http: allow url and options to be passed to http.request #21616

rubys commented Jul 1, 2018

mscdex Jul 1, 2018

mscdex Jul 1, 2018

vsemozhetbyt Jul 1, 2018

mscdex Jul 1, 2018

mscdex Jul 1, 2018

rubys commented Jul 1, 2018

vsemozhetbyt commented Jul 1, 2018

vsemozhetbyt commented Jul 1, 2018

mscdex Jul 1, 2018 •

edited

Loading

rubys Jul 1, 2018

rubys Jul 1, 2018

vsemozhetbyt Jul 1, 2018

mscdex Jul 1, 2018

mscdex Jul 1, 2018

vsemozhetbyt Jul 1, 2018

vsemozhetbyt Jul 1, 2018

vsemozhetbyt Jul 1, 2018

rubys commented Jul 1, 2018

Trott commented Jul 1, 2018

vsemozhetbyt Jul 1, 2018

vsemozhetbyt Jul 1, 2018

vsemozhetbyt Jul 1, 2018

mscdex Jul 2, 2018

TimothyGu Jul 3, 2018

rubys Jul 3, 2018

rubys commented Jul 26, 2018

http: allow url and options to be passed to http.request #21616

http: allow url and options to be passed to http.request #21616

Conversation

rubys commented Jul 1, 2018

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubys commented Jul 1, 2018

vsemozhetbyt commented Jul 1, 2018

vsemozhetbyt commented Jul 1, 2018

mscdex Jul 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubys commented Jul 1, 2018

Trott commented Jul 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubys commented Jul 26, 2018

mscdex Jul 1, 2018 •

edited

Loading