Updates to unblock Node 8 #929
Conversation
| @@ -52,7 +56,8 @@ function setRequestHeaders(req, options, interceptor) { | |||
| // We mock request headers if these were specified. | |||
| if (interceptor.reqheaders) { | |||
| _.forOwn(interceptor.reqheaders, function(val, key) { | |||
| setHeader(req, key, val); | |||
| if (!_.isFunction(val)) | |||
There was a problem hiding this comment.
What is val if it's a function and is it OK that we're not setting it?
There was a problem hiding this comment.
val can be the matching function from reqheaders. During the matching phase, headers specified in reqheaders are copied to the client request object - now using the setHeader() function. setHeader() checks for valid characters in the header value, so it errors when the value is a function.
I am actually not sure why the headers are copied from the interceptor to the request object, since I thought reqheaders are used only for matching. In any case, setting the function as a header value seems unintended, and taking it out did not break any tests.
|
Hi @martinkuba! Thanks so much for your contribution. I've been on vacation for the past month and a half an the other maintainers no longer appear to be working on the project. Have you had success running this code in your own projects? |
|
@ianwsperber Yes, we have been running our tests on my fork since I created this PR, and have not had any issues. We run our tests on all major versions of Node back to 0.8 (in https://github.com/newrelic/node-newrelic). |
|
Great. This has been an open issue for a long time so I'm going to move ahead with merging your PR. Thanks a ton for taking the time to get it in @martinkuba . Should have a patch up on NPM within a couple hours. |
|
@martinkuba unfortunately tests are failing on master ATM, those need to be fixed before I can go ahead with release. Will create issue to track. |
|
@ianwsperber Thanks for merging this. I wanted to reply to your other question, but did not get to it in time. I will follow up and look at the failing test as well. |
|
Opened #948 to fix the failing test. |
| }; | ||
|
|
||
| module.request = function(options, callback) { | ||
| // debug('request options:', options); | ||
| return newRequest(proto, overriddenRequest.bind(module), options, callback); | ||
| }; | ||
|
|
||
| if (semver.satisfies(process.version, '>=8')) { |
There was a problem hiding this comment.
This could be tested once in a shared include file, not on every request.
The dependency could be avoided by writing it as if (parseInt(process.version.slice(1) >= 8)
| } | ||
| }; | ||
|
|
||
| // in Node 8, res.end() does not call res.write() directly | ||
| if (semver.satisfies(process.version, '>=8')) { |
There was a problem hiding this comment.
see above re testing the condition only once
| @@ -34,8 +34,12 @@ function setHeader(request, name, value) { | |||
| request._headerNames = request._headerNames || {}; | |||
| request._removedHeader = request._removedHeader || {}; | |||
|
|
|||
| request._headers[key] = value; | |||
| request._headerNames[key] = name; | |||
| if (request.setHeader) { | |||
There was a problem hiding this comment.
even node v4 has a setHeader method on req, but v7 could still set directly.
Could just reuse the is-node-v8 condition computed above.
9.0.14 includes nock/nock#929 which fixes nock/nock#922 and nock/nock#925 which prevented nock from working on Node 8 and later. Now test everything on all supported Node versions. Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you! |
We are currently blocked from full support of Node 8 due to nock. This is my attempt to get nock working on Node 8. If someone can review and give me pointers, I am willing to finish this up.
There is one browserify test failing on Node 7 and 8, but it seems to have been there before this change.