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
Update/clarify interceptor.reply param juggling. #1520
Conversation
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.
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? |
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? |
Yep, I'd be happy to this weekend. To clarify expectations:
... and for all the callback versions, support async error-first callbacks too. |
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:
In terms of the cases you laid out above:
👍
Keep the provided status code and send the array as the body (breaking change)
Throw an error (is this a breaking change?)
👍
I'm good with throwing an error for |
I pushed changes to this branch adding a few additional asserts to the tests. I also added tests of the |
@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. |
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 |
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.
@paulmelnikow this could use some 👀 on it at this point. |
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.
🙌Awesome work! This is a bear of an effort. I left you a bunch of comments.
lib/common.js
Outdated
return input | ||
} | ||
|
||
const int = Number.parseInt(input) |
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 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.
lib/interceptor.js
Outdated
// 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 |
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.
// 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 |
lib/interceptor.js
Outdated
} | ||
this.statusCode = null | ||
this.callback = statusCode | ||
this.fullResponseFromCallback = true |
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.
👍
lib/interceptor.js
Outdated
@@ -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? |
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.
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.
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.
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.
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.
Ah I see. Makes sense: you're saying we want rawHeaders
to keep their original case.
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 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.
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.
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']
]
This PR probably resolves the issue #1208. |
@paulmelnikow I added changes based on your feedback today. Probably good for another review at this point. |
I'm going to try to get back to this later today or tomorrow. Apologies for the delay! |
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 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/request_overrider.js
Outdated
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? |
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 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 ? |
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've included this comment update, but I'm not sure that's the right issue at this point.
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! |
@paulmelnikow this looks gtg now. |
Matt, this was amazing work! Thank you so much! 🙌 |
🎉 This PR is included in version 11.0.0-beta.15 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Hey Matt, would you like to join as a maintainer? |
Sure. I use the lib a good bit at work, so I have a vested interest. |
🎉 This PR is included in version 11.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Based on this conversation.
The
arguments.length <= 2
was confusing/misleading, and clearing therawHeaders
clarifies the method's intention to anyone digging around,plus reduces the changes for bugs in the future.
Closes #1222.