fix(intercept): Add support for node 10.x ClientRequest #1428

nijynot · 2019-02-02T16:17:02Z

Fixes #1227 and #1239.
Added two tests for the support of the new ClientRequest.

paulmelnikow

Hi! Thanks so much for this contribution. I left some comments about it.

paulmelnikow · 2019-02-02T16:31:25Z

lib/intercept.js

@@ -235,6 +235,27 @@ function ErroringClientRequest(error) {

 inherits(ErroringClientRequest, http.ClientRequest)

+function urlToOptions(input) {


Is there some API and/or source code reference for this translation?

Yes there is, it's from the Node.js' internal/url.js file. https://github.com/nodejs/node/blob/master/lib/internal/url.js#L1257

Added some comment to the urlToOptions function, and the link.

paulmelnikow · 2019-02-02T16:32:45Z

lib/intercept.js

@@ -256,6 +277,16 @@ function overrideClientRequest() {
      )
    }

+    if (typeof input === 'string' && typeof options === 'object') {


Am I correct this is this a Node-10-or-later feature? I wonder, is there harm in our supporting this in earlier versions of Node? I feel like that could lead to confusing disparity between nock and real ClientRequest. Perhaps we should conditionally support the new form when running in Node 10+.

Just checked on ClientRequest in Node master, it seems they do a typeof check in a similar way, and then re-assigning the input to options. I think it should be backwards compatible?
https://github.com/nodejs/node/blob/master/lib/_http_client.js#L63

I guess the question is: what happens if you're in Node 8 and you pass a string to a real ClientRequest? The behavior from ours should be the same.

@paulmelnikow Ah, I see. I fixed it so that the Node 10 feature only work on Node 10 (see the requestParameters function). But I think this introduces some other problems, like testing. How do you test a Node 10 feature if you're on Node 8? Do we have a copy of Node 10 ClientRequest & Node <10 ClientRequest in nock so that tests work regardless of which Node you're testing with?

I guess easy solution would be to instead warn if someone uses Node 10 API when running on Node <10.

Edit: Nvm, ignore this

paulmelnikow · 2019-02-02T16:33:48Z

lib/intercept.js

+  if (input.username || input.password) {
+    options.auth = `${input.username}:${input.password}`
+  }
+  return options


It would be good to include tests to cover the rest of this function. It might be tempting to write these as unit tests, though we prefer to write functional tests via the public API because it helps ensure that all the code in the library is demanded by a real use case.

Added some more tests, specifically, tests that use http.request and http.get, which is what others will actually use.

paulmelnikow · 2019-02-02T20:03:16Z

lib/common.js

+ * - Utility function that converts a URL object into an ordinary
+ *   options object as expected by the http.request and https.request
+ *   APIs.
+ * - https://github.com/nodejs/node/blob/7237eaa3353aacf284289c8b59b0a5e0fa5744bb/lib/internal/url.js#L1257


👍

@gr2m Since this is an internal Node API I think it makes sense to vendor in this function. Do you know if we need to acknowledge it somewhere besides what's done here, in order to comply with the license?

paulmelnikow · 2019-02-02T20:04:25Z

lib/common.js

@@ -94,7 +124,17 @@ const overrideRequests = function(newRequest) {
      get: overriddenGet,
    }

-    module.request = function(options, callback) {
+    module.request = function(input, options, callback) {


So we don't end up with three copies of this function, could you extract it to a helper function in common.js?

paulmelnikow · 2019-02-02T20:16:31Z

Just a heads up that there are many lines of tests being moved in #1426 and it's a bit high risk, so I want to get that merged first. The tests here may need to be rebased after that lands.

paulmelnikow · 2019-05-06T00:16:18Z

lib/common.js

 */
-const overrideRequests = function(newRequest) {
+const overrideRequests = function(newRequest, legacy) {


Hi! so sorry to be just now getting back to this!

I see you've added a parameter here, which it seems like must be set to true when running in Node 8 and false otherwise.

Could you do this in code instead, so the caller doesn't need figure out the correct value for this parameter? We used to have code that did semver.satisfies(process.version, '>=8') and I think the same approach would work well here (with '<10').

~~You can look at #1318 where one of these conditional checks was removed.~~

I see elsewhere you've added auto detection using ClientRequest.length.

I don't think this behavior should be, or needs to be, configurable. Could you export a single constant in common.js and condition everything on that? I am hoping it can tighten this up.

paulmelnikow · 2019-05-06T00:22:20Z

lib/intercept.js

@@ -298,12 +307,73 @@ function overrideClientRequest() {
      }
    }
  }
-  inherits(OverriddenClientRequest, http.ClientRequest)
+  function OverriddenClientRequest(input, options, cb) {


There is a lot of duplicated code here. If the legacy behavior is conditioned on a constant exported from common.js, could this be handled more simply, e.g. with an if statement around the part which differs?

paulmelnikow · 2019-05-06T00:24:20Z

I tried to update your branch with latest beta and solve the merge conflict, but I don't have push access.

paulmelnikow · 2019-05-06T11:55:14Z

Pushed, thanks! By the way, GitHub provides a check box which makes it possible to give push access to the PR branch. That's an easier way to collaborate on PRs!

nijynot · 2019-05-06T12:36:58Z

By the way, GitHub provides a check box which makes it possible to give push access to the PR branch

Oh, didn't know that, will keep that in mind. Definitely easier to collaborate that way. 😆

I'll also take a closer look at the PR again. I remember getting stuck at some things, but will try to figure them out this time in the following days, I hope.

paulmelnikow · 2019-06-08T10:37:25Z

@nijynot Would you have some time to look at this? If not, no problem, I or one of the other maintainers could try to pick it up.

nijynot · 2019-06-09T09:56:38Z

@paulmelnikow Yeah, would be better if you or some other maintainer picked it up instead so I don't block.
I'll leave it as it is, so anyone is free to pick it up.

mastermatt · 2019-06-10T20:40:40Z

I've looked into this issue a bit already. I'll take a stab at it and see what happens.

paulmelnikow · 2019-07-07T17:31:12Z

Closing in favor of #1588.

nijynot mentioned this pull request Feb 2, 2019

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

Closed

paulmelnikow reviewed Feb 2, 2019

View reviewed changes

weekly-digest bot mentioned this pull request Feb 3, 2019

Weekly Digest (27 January, 2019 - 3 February, 2019) #1437

Closed

nijynot force-pushed the beta branch from 6e5ded9 to a612abe Compare February 3, 2019 12:16

gr2m mentioned this pull request Feb 3, 2019

Bug: The "listener" argument must be of type Function. Received type object #1239

Closed

paulmelnikow mentioned this pull request Feb 3, 2019

fix(intercept): Better error message when options is falsy #1440

Merged

nijynot force-pushed the beta branch from 7aeb459 to 20d795b Compare February 5, 2019 12:16

weekly-digest bot mentioned this pull request Feb 10, 2019

Weekly Digest (3 February, 2019 - 10 February, 2019) #1447

Closed

nijynot added 2 commits February 10, 2019 12:12

fix(intercept): Add support for node 10.x ClientRequest

b4e906f

fix(common): add helper function requestParameters

97a6d24

nijynot force-pushed the beta branch 2 times, most recently from 542fe39 to 40aff4d Compare February 10, 2019 11:57

add(intercept): support both node <10.9 and >=10.9 ClientRequest

9b80923

nijynot force-pushed the beta branch from 40aff4d to 9b80923 Compare February 11, 2019 19:39

weekly-digest bot mentioned this pull request Feb 17, 2019

Weekly Digest (10 February, 2019 - 17 February, 2019) #1450

Closed

paulmelnikow added this to In progress in 11.x via automation May 2, 2019

paulmelnikow reviewed May 6, 2019

View reviewed changes

Merge branch 'beta' into nijynot-beta

b204b74

weekly-digest bot mentioned this pull request May 12, 2019

Weekly Digest (5 May, 2019 - 12 May, 2019) #1554

Closed

paulmelnikow mentioned this pull request Jun 8, 2019

11.x release plan #1516

Closed

6 tasks

weekly-digest bot mentioned this pull request Jun 16, 2019

Weekly Digest (9 June, 2019 - 16 June, 2019) #1587

Closed

mastermatt mentioned this pull request Jun 17, 2019

feat: Support http.request signatures added in Node 10.9+ #1588

Merged

paulmelnikow closed this Jul 7, 2019

11.x automation moved this from In progress to Done Jul 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(intercept): Add support for node 10.x ClientRequest #1428

fix(intercept): Add support for node 10.x ClientRequest #1428

nijynot commented Feb 2, 2019

paulmelnikow left a comment

paulmelnikow Feb 2, 2019

nijynot Feb 2, 2019

paulmelnikow Feb 2, 2019

nijynot Feb 2, 2019

paulmelnikow Feb 2, 2019

nijynot Feb 3, 2019 •

edited

paulmelnikow Feb 2, 2019 •

edited

nijynot Feb 2, 2019

paulmelnikow Feb 2, 2019

paulmelnikow Feb 2, 2019

paulmelnikow commented Feb 2, 2019

paulmelnikow May 6, 2019 •

edited

paulmelnikow May 6, 2019

paulmelnikow commented May 6, 2019

paulmelnikow commented May 6, 2019 •

edited

nijynot commented May 6, 2019

paulmelnikow commented Jun 8, 2019

nijynot commented Jun 9, 2019

mastermatt commented Jun 10, 2019

paulmelnikow commented Jul 7, 2019

		@@ -235,6 +235,27 @@ function ErroringClientRequest(error) {

		inherits(ErroringClientRequest, http.ClientRequest)

		function urlToOptions(input) {

fix(intercept): Add support for node 10.x ClientRequest #1428

fix(intercept): Add support for node 10.x ClientRequest #1428

Conversation

nijynot commented Feb 2, 2019

paulmelnikow left a 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

nijynot Feb 3, 2019 • edited

Choose a reason for hiding this comment

paulmelnikow Feb 2, 2019 • edited

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

paulmelnikow commented Feb 2, 2019

paulmelnikow May 6, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulmelnikow commented May 6, 2019

paulmelnikow commented May 6, 2019 • edited

nijynot commented May 6, 2019

paulmelnikow commented Jun 8, 2019

nijynot commented Jun 9, 2019

mastermatt commented Jun 10, 2019

paulmelnikow commented Jul 7, 2019

nijynot Feb 3, 2019 •

edited

paulmelnikow Feb 2, 2019 •

edited

paulmelnikow May 6, 2019 •

edited

paulmelnikow commented May 6, 2019 •

edited