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
feat: Support http.request signatures added in Node 10.9+ #1588
feat: Support http.request signatures added in Node 10.9+ #1588
Conversation
826d859
to
3efe155
Compare
This is great! I can give it a read tomorrow. |
aa4d82c
to
9a7ec1b
Compare
Instead of passing it all around the overrider. This mimics how the native implementation does it.
Fixes nock#1227 Beginning with v10.9.0, the `url` parameter can be passed along with a separate options object to `http[s].request`, `http[s].get`, or to the constructor of `ClientRequest`. https://nodejs.org/api/http.html#http_http_request_url_options_callback The logic that does the juggling was mostly copied from Node's source.
9a7ec1b
to
fd54a81
Compare
@paulmelnikow were you able to read through this pr? |
Ah no, I think I lost track of it. Sorry! I'll aim to take a look tonight or tomorrow! |
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.
Thanks so much for this work! I understand most of it, I think, though I left a few questions.
I'll take another pass through thinking about test coverage though this is looking really good.
Sorry it's taken me so long to read this!
|
||
// The option per the docs is `protocol`. Its unclear if this line is meant to override that and is misspelled or if | ||
// the intend is to explicitly keep track of which module was called using a separate name. | ||
// Either way, `proto` is used as the source of truth from here on out. |
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.
Not sure what we ought to do here but really appreciate this comment!
res.on('response', callback) | ||
} | ||
return req | ||
return new http.ClientRequest(options, callback) |
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.
Nice!
Object.assign(this, overrider) | ||
|
||
if (callback) { | ||
this.once('response', callback) |
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.
Nice!
Co-Authored-By: Paul Melnikow <github@paulmelnikow.com>
@@ -453,3 +453,28 @@ test('percentEncode encodes extra reserved characters', t => { | |||
t.equal(common.percentEncode('foo+(*)!'), 'foo%2B%28%2A%29%21') | |||
t.done() | |||
}) | |||
|
|||
test('normalizeClientRequestArgs throws for invalid URL', async t => { |
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.
@gr2m has expressed a preference for testing our helper functions through real use cases in the public API wherever possible, and it seems to pay off well. It also has the advantage of surfacing code that is no longer needed.
Would you be up for expressing these tests in terms of calls to http
?
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 please 👍
scope.done() | ||
t.done() | ||
}) | ||
}) |
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.
👍
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.
Could you update the docs to mention the new request signature?
This PR doesn't change the documented API, rather it makes the mock surface compatible with Node 10.9+ (#1227). I don't think we have documentation of the supported mock surface, beyond the tests. We could add some, though I don't think it needs to be in this PR. |
Okay, thank you for the clarification |
I believe I've addressed all the comments. |
Ref #1404 (comment) This leaves intact two missed coverage spots in `lib/recorder.js` which are being taken care of in #1588. Also ref #1607
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.
Feel free to merge when ready!
🎉 This PR is included in version 11.0.0-beta.22 🎉 The release is available on: Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 11.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Ref #1404 (comment) This leaves intact two missed coverage spots in `lib/recorder.js` which are being taken care of in #1588. Also ref #1607
* Allow three arg form of `request` and `ClientRequest`. Fixes #1227 Beginning with v10.9.0, the `url` parameter can be passed along with a separate options object to `http[s].request`, `http[s].get`, or to the constructor of `ClientRequest`. https://nodejs.org/api/http.html#http_http_request_url_options_callback The logic that does the juggling was mostly copied from Node's source. * Remove unnecessary overrider in intercept. * Register ClientRequest callback on response event. Instead of passing it all around the overrider. This mimics how the native implementation does it.
Ref #1404 (comment) This leaves intact two missed coverage spots in `lib/recorder.js` which are being taken care of in #1588. Also ref #1607
* Allow three arg form of `request` and `ClientRequest`. Fixes #1227 Beginning with v10.9.0, the `url` parameter can be passed along with a separate options object to `http[s].request`, `http[s].get`, or to the constructor of `ClientRequest`. https://nodejs.org/api/http.html#http_http_request_url_options_callback The logic that does the juggling was mostly copied from Node's source. * Remove unnecessary overrider in intercept. * Register ClientRequest callback on response event. Instead of passing it all around the overrider. This mimics how the native implementation does it.
Ref #1404 (comment) This leaves intact two missed coverage spots in `lib/recorder.js` which are being taken care of in #1588. Also ref #1607
* Allow three arg form of `request` and `ClientRequest`. Fixes #1227 Beginning with v10.9.0, the `url` parameter can be passed along with a separate options object to `http[s].request`, `http[s].get`, or to the constructor of `ClientRequest`. https://nodejs.org/api/http.html#http_http_request_url_options_callback The logic that does the juggling was mostly copied from Node's source. * Remove unnecessary overrider in intercept. * Register ClientRequest callback on response event. Instead of passing it all around the overrider. This mimics how the native implementation does it.
) | ||
// https://nodejs.org/api/http.html#http_http_get_options_callback | ||
module.get = function(input, options, callback) { | ||
const req = module.request(input, options, callback) |
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.
Why is this not calling newRequest instead? This breaks the Node API as well
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.
This is done to mimic native Node behavior in case other libs are monkey-patching the function.
Can you provide more details on "This breaks the Node API as well"? Preferably in a new issue.
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.
I can make a new issue. But writing the details here as well.
New Relic (https://github.com/newrelic/node-newrelic) is overwriting http.get and http.request like you do. But when http.get is calling http.request like here, a call to http.get will call http.request twice when using nock in a New Relic test.
To prove that this is not native Node behaviour (at least on Node 12):
const http = require('http');
function request() {
console.log('http.request', arguments);
}
http.request = request;
// our request function is not called
http.get('http://google.com', () => {});
Fixes #1227
Supersedes #1428
Beginning with v10.9.0, the
url
parameter can be passed along with aseparate options object to
http[s].request
,http[s].get
, or to theconstructor of
ClientRequest
. docsThe logic that does the juggling was mostly copied from Node's source.
There was a bit of other cleanup that was committed separately to aid with review.