http: allow url and options to be passed to http.request by rubys · Pull Request #21616 · nodejs/node · GitHub

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

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

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

be an a -> be a

s/if/If/

s/options/`options`/

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

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

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

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

Ditto about REPLACEME

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()).

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

be an a -> be a

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

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() .

@nodejs/http

http -> https

Should there also be 2 headings?

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

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

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?

done

fixes nodejs#20795

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

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

targos mentioned this pull request Jul 26, 2018

tools: produce JSON documentation using unified/remark/rehype #21697

Closed

2 tasks

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

http: allow url and options to be passed to http.request by rubys · Pull Request #21616 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

http: allow url and options to be passed to http.request#21616rubys wants to merge 2 commits intonodejs:masternodejs/node:masterfrom rubys:issue-20795rubys/node:issue-20795Copy head branch name to clipboard

Conversation

rubys commented Jul 1, 2018

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rubys commented Jul 1, 2018

Uh oh!

vsemozhetbyt commented Jul 1, 2018

Uh oh!

vsemozhetbyt commented Jul 1, 2018

Uh oh!

mscdex Jul 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rubys commented Jul 1, 2018

Uh oh!

Trott commented Jul 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rubys commented Jul 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

http: allow url and options to be passed to http.request#21616
rubys wants to merge 2 commits into
nodejs:masterfrom
rubys:issue-20795

mscdex Jul 1, 2018 •
edited
Loading