Update/clarify interceptor.reply param juggling. #1520

mastermatt · 2019-05-01T20:00:14Z

The arguments.length <= 2 was confusing/misleading, and clearing the
rawHeaders clarifies the method's intention to anyone digging around,
plus reduces the changes for bugs in the future.

Closes #1222.

Based on conversation https://github.com/nock/nock/pull/1517/files#r280139478. The `arguments.length <= 2` was confusing/misleading, and clearing the `rawHeaders` clarifies the method's intention to anyone digging around, plus reduces the changes for bugs in the future.

lib/interceptor.js

paulmelnikow · 2019-05-01T20:09:24Z

Hi @mastermatt, thanks so much for picking this up! 🙌

I've got a branch going that reworks a bunch of tests related to this. So far I haven't implemented any changes in behavior changes, however I've identified a few bugs and fairly confusing edge cases.

It might make sense to review and merge that first, and then add / change those tests to document the behavior changes being made here.

I should have the PR up momentarily. When it is, would you mind taking a look and seeing what you think?

paulmelnikow · 2019-05-03T20:59:44Z

Hey @mastermatt I merged the additional tests. Do you want to make the breaking change we discussed at #1521 (review) and fix the bug #1521 (comment)? Could you merge master into this branch?

mastermatt · 2019-05-03T22:03:25Z

Yep, I'd be happy to this weekend.

To clarify expectations:

.reply(status) -- no change
.reply(status, nonCallbackBody, [headers]) -- no change
.reply(status, () => nonArray, [headers]) -- no change
.reply(status, () => Array, [headers]) -- change: throw an error
.reply(() => nonArray) -- no change, status 200
.reply(() => Array) -- use the values as status, body, headers.
- Bugfix: ensure the defaults are [200, '', {}].
- Should an error be thrown if the array has length of zero or > 3?

... and for all the callback versions, support async error-first callbacks too.

paulmelnikow · 2019-05-05T23:57:55Z

My preference on the API is slightly different. I'd really like to reduce the amount of magic in these calls. While I appreciate flexibility, I'd rather do it without so much magic, as the magic leads to unexpected behaviors 😀

Here's what I'd propose:

When a status code is provided followed by a function, the function returns the body. No magic.
When a function is provided on its own, it must return an array of status, [body, [headers]]. Again, no magic.

In terms of the cases you laid out above:

.reply(status) -- no change

.reply(status, nonCallbackBody, [headers]) -- no change

.reply(status, () => nonArray, [headers]) -- no change

👍

.reply(status, () => Array, [headers]) -- change: throw an error

Keep the provided status code and send the array as the body (breaking change)

.reply(() => nonArray) -- no change, status 200

Throw an error (is this a breaking change?)

.reply(() => Array) -- use the values as status, body, headers.

👍

Bugfix: ensure the defaults are [200, '', {}].

Should an error be thrown if the array has length of zero or > 3?

I'm good with throwing an error for .reply(() => []) or when the length is > 3. Both of those cases seem more likely to be a mistake than deliberate. Definitely good with empty body as a default.

paulmelnikow · 2019-05-06T00:01:58Z

I pushed changes to this branch adding a few additional asserts to the tests. I also added tests of the .reply() and reply(201) cases.

mastermatt · 2019-05-06T01:47:34Z

@paulmelnikow your version will have a larger breaking change, but I think it's the right direction and if you're good with it, I'm good with it.

paulmelnikow · 2019-05-06T02:48:27Z

Cool. I think it's the right direction too. And I feel good about it (though may want to give it one last thought after seeing how it changes the behavior as expressed in actual tests).

It goes with the thrust of #1170, which is that .reply(() => [417]) should trigger a 417 status code. I just did some more digging re #1203 and #1222 and posted #1203 (comment) arguing further that .reply(() => [417]) should send an empty response, rather then sending [417].

This commit only includes the implementation and tests. Callers are to follow, but I didn’t want to muddy commit history.

Includes breaking changes. - When a status code is provided followed by a function, the function returns the body. No magic. - When a function is provided on its own, it _MUST_ return an array of `status, [body, [headers]]`. Again, no magic. This change uses errors to enforce the following two signatures: `.reply(status, [body, [headers]])` where `body` is any of the existing types that can result in a scalar value. `.reply(() => [status, body = ‘’, headers = {}])` where the callback is called using the existing mechanics. #### Breaking changes - There is no longer a default value for the response status code. It must be provided as the first arg of `reply` or the first element in the returned array if using the single-function style. - `.reply()` will now throw an error - Status code args will throw an error if they cannot be cast to ints. - The single-function style callback **MUST** return an array with a length of 1-3 or else an error is thrown. - If **NOT** using the single-function style callback, the resulting body will never be treated as a special value for status code or headers, it will always result to the responses body value. - `reply(200, () => [400, ‘foo’])` - **previous result** status: 400 body: ‘foo’ - **new result** status: 200 body: “[400, ‘foo’]” - `reply(200, () => [400])` - **previous result** status: 400 body: “[400]” - **new result** status: 200 body: “[400]”

Calling `filteringPath` on the intercept instance was broken as the transform fn set on the scope had the wrong name. Found when looking at Uncovered lines in coveralls.

mastermatt · 2019-05-06T18:23:39Z

@paulmelnikow this could use some 👀 on it at this point.
9913f68 is the meat.

paulmelnikow

🙌Awesome work! This is a bear of an effort. I left you a bunch of comments.

lib/common.js

paulmelnikow · 2019-05-06T19:25:09Z

lib/common.js

+    return input
+  }
+
+  const int = Number.parseInt(input)


I appreciate your thoroughness!

Is there any reason we need to accept anything other than an integer here?

If we don't need to, I'd rather not. It's more behavior to test and more code to maintain.

paulmelnikow · 2019-05-06T19:27:06Z

lib/interceptor.js

+  // support the format of only passing in a callback
+  if (_.isFunction(statusCode)) {
+    if (arguments.length > 1) {
+      // it's not very Javascript-y to throw an error for this kind of thing, but because


Suggested change

// it's not very Javascript-y to throw an error for this kind of thing, but because

// It's not very Javascript-y to throw an error for extra args to a function, but because

paulmelnikow · 2019-05-06T19:27:30Z

lib/interceptor.js

+    }
+    this.statusCode = null
+    this.callback = statusCode
+    this.fullResponseFromCallback = true


paulmelnikow · 2019-05-06T19:30:01Z

lib/interceptor.js

@@ -94,6 +107,7 @@ Interceptor.prototype.reply = function reply(statusCode, body, rawHeaders) {

  if (this.scope._defaultReplyHeaders) {
    headers = headers || {}
+    // because of this, this.rawHeaders gets lower-cased versions of the `rawHeaders` param. is that a bug?


If I understand your question correctly, no, it's not a bug, and there are tests ensuring that this behavior is followed. The field names of HTTP headers are case-insensitive, and nock lowercases everything for matching.

The headers object is meant to be case-insensitive by always holding the lowercase version of the keys, however, the case-sensitivity of rawHeaders is inconsistent.
Usually they're kept as the provided value, but sometimes (like here) they get lowercased first.
I'm not positive what the intent is, I've just added a couple comments about the inconsistency when I come across them.

Ah I see. Makes sense: you're saying we want rawHeaders to keep their original case.

I don't use rawHeaders in any of my projects and the README never mentions them. So I'm not sure what we want.
If the intention is to mimic http.IncomingMessage.rawHeaders then it seems the current implementation is buggy.

The raw request/response headers list exactly as they were received.
...
Header names are not lowercased, and duplicates are not merged.
...
For example, the previous message header object might have a rawHeaders list like the following:

[ 'ConTent-Length', '123456', 'content-LENGTH', '123', 'content-type', 'text/plain', 'CONNECTION', 'keep-alive', 'Host', 'mysite.com', 'accepT', '*/*' ]

This topic should probably be moved to its own issue if we consider it a bug.

If the intention is to mimic http.IncomingMessage.rawHeaders then it seems the current implementation is buggy.

Agreed, let's open a new issue. It sounds like it may require some thinking through.

We do want the rawHeaders on a nock-produced IncomingMessage to match what they would be on a real one, and ideally it should be possible to establish through nock whatever casing the caller might want.

Interestingly, I don't think that is the format of our rawHeaders object. I think at least one of our tests for rawHeaders is showing an array for the duplicated headers:

[ 'content-length': ['123456', '123'] ]

tests/test_intercept.js

tests/test_repeating.js

tests/test_reply_body.js

tests/test_reply_function_async.js

tests/test_reply_headers.js

modestfake · 2019-05-07T17:52:46Z

This PR probably resolves the issue #1208.

mastermatt · 2019-05-09T02:34:53Z

@paulmelnikow I added changes based on your feedback today. Probably good for another review at this point.

paulmelnikow · 2019-05-16T17:34:32Z

I'm going to try to get back to this later today or tomorrow. Apologies for the delay!

paulmelnikow

This looks awesome, Matt! Thank you so much for taking this on.

I left a handful of basically trivial changes. Want to go through and apply those and then I'll merge?

lib/interceptor.js

lib/request_overrider.js

paulmelnikow · 2019-05-19T19:58:03Z

lib/request_overrider.js

          if (typeof responseBody === 'string') {
            responseBody = Buffer.from(responseBody)
          } else {
            responseBody = JSON.stringify(responseBody)
            response.headers['content-type'] = 'application/json'
          }
        }
+        // why are strings converted to a Buffer, but JSON data is left as a string? related #1542?


Suggested change

// why are strings converted to a Buffer, but JSON data is left as a string? related #1542?

// why are strings converted to a Buffer, but JSON data is left as a string?

// Related to https://github.com/nock/nock/issues/1542 ?

I've included this comment update, but I'm not sure that's the right issue at this point.

lib/scope.js

tests/test_reply_body.js

tests/test_reply_function_async.js

tests/test_reply_headers.js

tests/test_stream.js

paulmelnikow · 2019-05-19T20:21:07Z

I was thinking that we might need to update the usage instructions in the readme, though looking at it, I think we have just made it match what's there less ambiguously. I added a brief note explaining what changed between 11.x and 12.x. Please take a look!

mastermatt · 2019-05-20T17:05:54Z

@paulmelnikow this looks gtg now.

paulmelnikow · 2019-05-20T17:20:27Z

Matt, this was amazing work! Thank you so much! 🙌

nockbot · 2019-05-20T17:25:35Z

🎉 This PR is included in version 11.0.0-beta.15 🎉

The release is available on:

Your semantic-release bot 📦🚀

paulmelnikow · 2019-05-20T17:39:31Z

Hey Matt, would you like to join as a maintainer?

mastermatt · 2019-05-20T17:46:40Z

Sure. I use the lib a good bit at work, so I have a vested interest.

nockbot · 2019-08-12T23:57:58Z

🎉 This PR is included in version 11.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

…1520) Ref https://github.com/nock/nock/pull/1517/files#r280139478 Closes #1222

mastermatt added 2 commits May 1, 2019 13:58

chore(docs): update JSDoc types in interceptor.

9b92974

mastermatt commented May 1, 2019

View reviewed changes

lib/interceptor.js Outdated Show resolved Hide resolved

paulmelnikow mentioned this pull request May 2, 2019

Improve tests of .reply() with a synchronous function #1521

Merged

paulmelnikow added this to To do in 11.x May 2, 2019

paulmelnikow moved this from To do to In progress in 11.x May 2, 2019

paulmelnikow added the breaking-change label May 2, 2019

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

Weekly Digest (28 April, 2019 - 5 May, 2019) #1539

Closed

paulmelnikow added 2 commits May 5, 2019 19:38

Merge branch 'beta' into update-interceptor-reply

6ba0f5c

Add explicit status code and body checks

4cae0ba

Add tests of reply() with no arguments

be2b310

paulmelnikow mentioned this pull request May 6, 2019

Fix test: "get with reply callback returning default statusCode without body" #1222

Closed

mastermatt added 5 commits May 6, 2019 10:43

Add statusCodeInt helper too common.

14fe6d0

This commit only includes the implementation and tests. Callers are to follow, but I didn’t want to muddy commit history.

Notes and tests about inconsistent raw headers.

7d1a3be

Bugfix Interceptor.filteringPath

5baf9ed

Calling `filteringPath` on the intercept instance was broken as the transform fn set on the scope had the wrong name. Found when looking at Uncovered lines in coveralls.

Add tests to increase coverage in new error flows.

833dae2

paulmelnikow reviewed May 6, 2019

View reviewed changes

mastermatt mentioned this pull request May 7, 2019

Bugfix Interceptor.filteringPath #1543

Merged

This was referenced May 7, 2019

Nock doesn't reply when data to be sent is false #1446

Closed

Unable to return an array as valid JSON response #1228

Closed

mastermatt mentioned this pull request May 11, 2019

Behavior of response.rawHeaders does not match native functionality #1553

Closed

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

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

Closed

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

Weekly Digest (12 May, 2019 - 19 May, 2019) #1560

Closed

paulmelnikow reviewed May 19, 2019

View reviewed changes

Add a note to the readme

7043064

CR feedback updates.

f317877

paulmelnikow approved these changes May 20, 2019

View reviewed changes

paulmelnikow merged commit 2e779f0 into nock:beta May 20, 2019

11.x automation moved this from In progress to Done May 20, 2019

mastermatt deleted the update-interceptor-reply branch May 20, 2019 17:23

nockbot added the released label May 20, 2019

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

Weekly Digest (19 May, 2019 - 26 May, 2019) #1567

Closed

mastermatt mentioned this pull request May 28, 2019

Add test to cover invalid reply status codes. #1571

Merged

This was referenced Jun 25, 2019

Arrays returned from interceptor functions are parsed as [statusCode, body] even if status Code is given in the call #1584

Closed

Bump nock from 11.0.0.beta-14 to 11.0.0.beta-20 badges/shields#3616

Merged

paulmelnikow mentioned this pull request Jul 8, 2019

Return Array from .reply(200, 'return array instead of object or string'). #1208

Closed

gr2m pushed a commit that referenced this pull request Sep 4, 2019

fix: Update and clarify how .reply() can be invoked with functions (#…

95c854d

…1520) Ref https://github.com/nock/nock/pull/1517/files#r280139478 Closes #1222

gr2m pushed a commit that referenced this pull request Sep 4, 2019

fix: Update and clarify how .reply() can be invoked with functions (#…

a98f71e

…1520) Ref https://github.com/nock/nock/pull/1517/files#r280139478 Closes #1222

nockbot mentioned this pull request Sep 4, 2019

The automated release is failing 🚨 #1704

Closed

gr2m pushed a commit that referenced this pull request Sep 5, 2019

fix: Update and clarify how .reply() can be invoked with functions (#…

ff88c1f

…1520) Ref https://github.com/nock/nock/pull/1517/files#r280139478 Closes #1222

paulmelnikow mentioned this pull request Sep 6, 2019

refactor: Refactor exports to reduce indirection #1710

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update/clarify interceptor.reply param juggling. #1520

Update/clarify interceptor.reply param juggling. #1520

mastermatt commented May 1, 2019 •

edited by paulmelnikow

paulmelnikow commented May 1, 2019

paulmelnikow commented May 3, 2019

mastermatt commented May 3, 2019

paulmelnikow commented May 5, 2019

paulmelnikow commented May 6, 2019

mastermatt commented May 6, 2019

paulmelnikow commented May 6, 2019

mastermatt commented May 6, 2019

paulmelnikow left a comment

paulmelnikow May 6, 2019

paulmelnikow May 6, 2019

paulmelnikow May 6, 2019

paulmelnikow May 6, 2019

mastermatt May 8, 2019

paulmelnikow May 8, 2019

mastermatt May 8, 2019

paulmelnikow May 8, 2019

modestfake commented May 7, 2019

mastermatt commented May 9, 2019

paulmelnikow commented May 16, 2019

paulmelnikow left a comment

paulmelnikow May 19, 2019

mastermatt May 20, 2019

paulmelnikow commented May 19, 2019

mastermatt commented May 20, 2019

paulmelnikow commented May 20, 2019

nockbot commented May 20, 2019

paulmelnikow commented May 20, 2019

mastermatt commented May 20, 2019

nockbot commented Aug 12, 2019

	// it's not very Javascript-y to throw an error for this kind of thing, but because
	// It's not very Javascript-y to throw an error for extra args to a function, but because

	// why are strings converted to a Buffer, but JSON data is left as a string? related #1542?
	// why are strings converted to a Buffer, but JSON data is left as a string?
	// Related to https://github.com/nock/nock/issues/1542 ?

Update/clarify interceptor.reply param juggling. #1520

Update/clarify interceptor.reply param juggling. #1520

Conversation

mastermatt commented May 1, 2019 • edited by paulmelnikow

paulmelnikow commented May 1, 2019

paulmelnikow commented May 3, 2019

mastermatt commented May 3, 2019

paulmelnikow commented May 5, 2019

paulmelnikow commented May 6, 2019

mastermatt commented May 6, 2019

paulmelnikow commented May 6, 2019

mastermatt commented May 6, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

modestfake commented May 7, 2019

mastermatt commented May 9, 2019

paulmelnikow commented May 16, 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

paulmelnikow commented May 19, 2019

mastermatt commented May 20, 2019

paulmelnikow commented May 20, 2019

nockbot commented May 20, 2019

paulmelnikow commented May 20, 2019

mastermatt commented May 20, 2019

nockbot commented Aug 12, 2019

mastermatt commented May 1, 2019 •

edited by paulmelnikow