-
Notifications
You must be signed in to change notification settings - Fork 84
refactor(lumberjack): replace with util fn #1213
Conversation
// In Node.js v8 and earlier https.request internally called http.request, but this is changed in | ||
// later versions | ||
// https://github.com/nodejs/node/blob/v6.x/lib/https.js#L206 | ||
// https://github.com/nodejs/node/blob/v8.x/lib/https.js#L239 | ||
// https://github.com/nodejs/node/blob/v10.x/lib/https.js#L271 | ||
if (Number.parseInt(/^v(\d+)/.exec(process.version)[1], 10) > 8) { | ||
monkeypatches.attachHttpsRequestSpy(outgoingRequestSpy, outgoingRequestEndSpy); | ||
} | ||
monkeypatches.attachHttpRequestSpy(outgoingRequestSpy, outgoingRequestEndSpy); |
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 has been simplified as One App has long since dropped support for node 8
@@ -0,0 +1,47 @@ | |||
/* |
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.
}; | ||
} | ||
|
||
export default function attachRequestSpies(requestSpy, socketCloseSpy) { |
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.
Size Change: 0 B Total Size: 712 kB ℹ️ View Unchanged
|
attachSpy(https, 'request', httpSpy('https', requestSpy, socketCloseSpy)); | ||
attachSpy(http, 'request', httpSpy('http', requestSpy, socketCloseSpy)); |
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 is the only change. instead of having a function for each attachment we have a single function for attaching both.
return url.parse(url.format(buildUrlObject(options, defaultProtocol))); | ||
} | ||
|
||
function httpSpy(defaultProtocol, requestSpy, socketCloseSpy) { |
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.
import attachSpy from './attachSpy'; | ||
|
||
function buildUrlObject(options, defaultProtocol) { | ||
// TODO: url.parse is deprecated, use new URL() instead |
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 change is beyond the scope of this PR
const urlObject = { | ||
auth: options.auth, | ||
hostname: options.hostname || options.host || 'localhost', | ||
protocol, | ||
port: options.port || (protocol === 'http:' ? 80 : 443), | ||
hash: parsedPath.hash, | ||
path: parsedPath.path, | ||
pathname: parsedPath.pathname, | ||
query: parsedPath.query, | ||
search: parsedPath.search, | ||
}; | ||
if ( | ||
(protocol === 'http:' && urlObject.port === 80) | ||
|| (protocol === 'https:' && urlObject.port === 443) | ||
) { | ||
urlObject.port = undefined; | ||
} | ||
return urlObject; | ||
} |
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.
(ref #1213 (comment)), knowing that we can use undefined
for port
, then there's no need for the if statement + mutation, we could simplify it like:
const urlObject = { | |
auth: options.auth, | |
hostname: options.hostname || options.host || 'localhost', | |
protocol, | |
port: options.port || (protocol === 'http:' ? 80 : 443), | |
hash: parsedPath.hash, | |
path: parsedPath.path, | |
pathname: parsedPath.pathname, | |
query: parsedPath.query, | |
search: parsedPath.search, | |
}; | |
if ( | |
(protocol === 'http:' && urlObject.port === 80) | |
|| (protocol === 'https:' && urlObject.port === 443) | |
) { | |
urlObject.port = undefined; | |
} | |
return urlObject; | |
} | |
const urlObject = { | |
auth: options.auth, | |
hostname: options.hostname || options.host || 'localhost', | |
protocol, | |
portx: options.port || (protocol === 'http:' ? 80 : 443), | |
port: options.port && ( | |
(protocol === 'http:' && options.port !== 80) || | |
(protocol === 'https:' && options.port !== 443) | |
) ? options.port : undefined, | |
hash: parsedPath.hash, | |
path: parsedPath.path, | |
pathname: parsedPath.pathname, | |
query: parsedPath.query, | |
search: parsedPath.search, | |
}; | |
return urlObject; | |
} |
The idea is that, if the options.port
is provided then we validate and make sure it's a different port (different than 80
for http
and different than 443
for https
), otherwise we just set it to undefined
.
Not, this leads me to the following question, why do we need to unset the port for those cases? can't just use 80
for http
since that's the provided port? looks like that we need it's just port: options.port
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 is not quite the same result. if the protocol is undefined and no options.port is set the current result would be 443
not undefined
. Im not convinced that this is the intended result but for this PR im trying to keep the functionality identical.
I think having the removal as a separate conditional is clearer. I suspect this logic is based around old Node edge cases.
This PR is intended as a simple lift and shift. This will need a rewrite as part of the removal url.parse
commit type should be |
will change on merge |
|
||
const callOriginal = jest.fn(() => 'client request object'); | ||
|
||
httpsSpy(['http://example.tld'], callOriginal); |
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.
Can we call from http directly?
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.
we could, these tests rely on attachSpy being mocked so they will all need a rewrite.
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.
Description
This is almost a direct lift and shift from https://github.com/americanexpress/lumberjack/blob/main/src/monkeypatches/attachHttpsRequestSpy.js.
For simplicity i combined the attach http and https request spies into single function.
Motivation and Context
One App is now only using small part of Lumberjack. This will allow lumberjack to be deprecated.
How Has This Been Tested?
Test suite.
Types of Changes
Checklist:
What is the Impact to Developers Using One App?