nock does not support the new Node.js 10.9 signature http.request(url[, options][, callback])

[gkalpak]

What is the expected behavior?
Using the new signature for http.request(url, options[, callback])/http.get(url, options[, callback]) (introduced in Node.js v10.9, should work with nock.

What is the actual behavior?
With nock (by merely requireing nock), it throws:

events.js:288
    throw new errors.ERR_INVALID_ARG_TYPE('listener', 'Function', listener);
    ^

TypeError [ERR_INVALID_ARG_TYPE]: The "listener" argument must be of type Function. Received type object
    at ClientRequest.once (events.js:288:11)
    at new ClientRequest (_http_client.js:150:10)
    at Object.request (http.js:41:10)
    at ...\node_modules\nock\lib\intercept.js:405:16
    at Object.module.request (...\node_modules\nock\lib\common.js:140:14)
    at requestWithUrlAndOptions (...\script.js:27:5)
    at Object.<anonymous> (...\script.js:42:1)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)

Possible solution
I haven't looked at the code, but I suspect nock assumes the second argument is always the callback and uses it as such, whereas in Node.js 10.9+, the second argument can be an object (with the third being the callback). It should be trivial to detect that situation and pass the correct arguments to http.request() (or something like that 😁).

How to reproduce the issue

Runkit: Example link

Does the bug have a test case?
(Not sure wht this means.)

Versions

Software	Version(s)
Nock	All (at least up to 10.0.0)
Node	10.9+

[gr2m]

I’m not familiar enough with the code base myself to tell what the problem could be. The runkit notebook is much appreciated, thank you!

Would you like to get a PR going? A failing test would be good enough, you can adjust the .travis.yml file to require node 10.9

[gkalpak]

I had a brief look at the source code and tests, but I couldn't figure out where the appropriate place would be (neither for the fix nor for the test) 😕
I am sure that it is a super simple and quick fix for someone familiar with the code base.

AFAICT, tests are already being run on Node.js 10 on Travis, so that is taken care of.

And as for a failing testcase, it is as simple as http.request('some url', {some: 'options'}, someCallback) (you can also see my runkit example). Again, I am not sure about the format of the tests, but it will be very straight forward for someone familiar with the code base.

[leonrinkel]

You are correct ✅

I used the following hack in common.js:138 to quickly get it running with the new signature:

module.request = function(url, options, callback) {
  url = new URL(url);

  options.protocol = url.protocol;
  options.hostname = url.hostname;
  options.port = url.port;
  options.path = url.pathname;

  return newRequest(proto, overriddenRequest.bind(module), options, callback);
};

Of course this is not a production-ready solution.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We try to do our best, but nock is maintained by volunteers and there is only so much we can do at a time. Thank you for your contributions.

[gr2m]

I fear nobody is super familiar with the code base right now, but we are working on cleaning that up. We just dropped support for Node < 10, come join the fun :)

[nijynot]

@gr2m I'll join in on the fun :-), just submitted a PR (#1428) that fixes this bug. Would be cool if someone could review it.

[nockbot]

🎉 This issue has been resolved in version 11.0.0-beta.22 🎉

The release is available on:

• npm package (@beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[nockbot]

🎉 This issue has been resolved in version 11.0.0 🎉

The release is available on:

• npm package (@next dist-tag)
• GitHub release

Your semantic-release bot 📦🚀