Skip to content
This repository was archived by the owner on Jan 21, 2026. It is now read-only.

Commit bd9863b

Browse files
kjinkjin
authored
fix: unpin @types/node and account for new http.request signatures (#1120)
1 parent 41a92f7 commit bd9863b

File tree

6 files changed

+103
-58
lines changed

6 files changed

+103
-58
lines changed

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,7 +65,7 @@
6565
"@types/mocha": "^5.2.5",
6666
"@types/mongoose": "^5.3.26",
6767
"@types/nock": "^10.0.0",
68-
"@types/node": "~10.7.2",
68+
"@types/node": "^12.7.2",
6969
"@types/node-fetch": "^2.5.0",
7070
"@types/once": "^1.4.0",
7171
"@types/proxyquire": "^1.3.28",

‎scripts/utils.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ function promisifyChildProcess(childProcess: ChildProcess): Promise<void> {
4444
});
4545
}
4646

47-
export function spawnP(command: string, args?: string[], options?: SpawnOptions): Promise<void> {
47+
export function spawnP(command: string, args: string[] = [], options: SpawnOptions = {}): Promise<void> {
4848
const stringifiedCommand = `\`${command}${args ? (' ' + args.join(' ')) : ''}\``;
4949
console.log(`> Running: ${stringifiedCommand}`);
5050
return promisifyChildProcess(spawn(command, args, Object.assign({
@@ -53,7 +53,7 @@ export function spawnP(command: string, args?: string[], options?: SpawnOptions)
5353
}, options)));
5454
}
5555

56-
export function forkP(moduleName: string, args?: string[], options?: ForkOptions): Promise<void> {
56+
export function forkP(moduleName: string, args: string[] = [], options: ForkOptions = {}): Promise<void> {
5757
const stringifiedCommand = `\`${moduleName}${args ? (' ' + args.join(' ')) : ''}\``;
5858
console.log(`> Running: ${stringifiedCommand}`);
5959
return promisifyChildProcess(fork(moduleName, args, Object.assign({

‎src/plugins/plugin-http.ts‎

Lines changed: 43 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -15,12 +15,11 @@
1515
*/
1616

1717
import * as httpModule from 'http';
18-
import {Agent, ClientRequest, ClientRequestArgs, get, request} from 'http';
18+
import {Agent, ClientRequest, ClientRequestArgs, request} from 'http';
1919
import * as httpsModule from 'https';
20-
import * as is from 'is';
2120
import * as semver from 'semver';
2221
import * as shimmer from 'shimmer';
23-
import * as url from 'url';
22+
import {URL, parse as urlParse} from 'url';
2423

2524
import {Plugin, Tracer} from '../plugin-types';
2625

@@ -31,16 +30,15 @@ type RequestFunction = typeof request;
3130
const ERR_HTTP_HEADERS_SENT = 'ERR_HTTP_HEADERS_SENT';
3231
const ERR_HTTP_HEADERS_SENT_MSG = "Can't set headers after they are sent.";
3332

34-
// tslint:disable:no-any
35-
const isString = is.string as (value: any) => value is string;
36-
// url.URL is used for type checking, but doesn't exist in Node <7.
33+
// URL is used for type checking, but doesn't exist in Node <7.
3734
// This function works around that.
35+
// tslint:disable:no-any
3836
const isURL = semver.satisfies(process.version, '>=7')
39-
? (value: any): value is url.URL => value instanceof url.URL
40-
: (value: any): value is url.URL => false;
37+
? (value: any): value is URL => value instanceof URL
38+
: (value: any): value is URL => false;
4139
// tslint:enable:no-any
4240

43-
function getSpanName(options: ClientRequestArgs | url.URL) {
41+
function getSpanName(options: ClientRequestArgs | URL) {
4442
// c.f. _http_client.js ClientRequest constructor
4543
return options.hostname || options.host || 'localhost';
4644
}
@@ -51,7 +49,7 @@ function getSpanName(options: ClientRequestArgs | url.URL) {
5149
* all-caps for simplicity purposes.
5250
* @param options Options for http.request.
5351
*/
54-
function hasExpectHeader(options: ClientRequestArgs | url.URL): boolean {
52+
function hasExpectHeader(options: ClientRequestArgs | URL): boolean {
5553
return !!(
5654
(options as ClientRequestArgs).headers &&
5755
((options as ClientRequestArgs).headers!.Expect ||
@@ -61,7 +59,7 @@ function hasExpectHeader(options: ClientRequestArgs | url.URL): boolean {
6159
}
6260

6361
function extractUrl(
64-
options: ClientRequestArgs | url.URL,
62+
options: ClientRequestArgs | URL,
6563
fallbackProtocol: string
6664
) {
6765
let path;
@@ -88,7 +86,7 @@ function extractUrl(
8886
}
8987

9088
// tslint:disable-next-line:no-any
91-
function isTraceAgentRequest(options: any, api: Tracer) {
89+
function isTraceAgentRequest(options: httpModule.RequestOptions, api: Tracer) {
9290
return (
9391
options &&
9492
options.headers &&
@@ -103,38 +101,52 @@ function makeRequestTrace(
103101
): RequestFunction {
104102
// On Node 8+ we use the following function to patch both request and get.
105103
// Here `request` may also happen to be `get`.
106-
return function requestTrace(options, callback): ClientRequest {
107-
if (!options) {
108-
return request(options, callback);
104+
return function requestTrace(
105+
this: never,
106+
url: httpModule.RequestOptions | string | URL,
107+
options?:
108+
| httpModule.RequestOptions
109+
| ((res: httpModule.IncomingMessage) => void),
110+
callback?: (res: httpModule.IncomingMessage) => void
111+
): ClientRequest {
112+
// These are error conditions; defer to http.request and don't trace.
113+
if (!url || (typeof url === 'object' && typeof options === 'object')) {
114+
return request.apply(this, arguments);
115+
}
116+
117+
let urlString;
118+
if (typeof url === 'string') {
119+
// save the value of uri so we don't have to reconstruct it later
120+
urlString = url;
121+
url = urlParse(url);
122+
}
123+
if (typeof options === 'function') {
124+
callback = options;
125+
options = url;
126+
} else {
127+
options = Object.assign({}, url, options);
109128
}
110129

111130
// Don't trace ourselves lest we get into infinite loops
112131
// Note: this would not be a problem if we guarantee buffering
113132
// of trace api calls. If there is no buffering then each trace is
114133
// an http call which will get a trace which will be an http call
115134
if (isTraceAgentRequest(options, api)) {
116-
return request(options, callback);
117-
}
118-
119-
let uri;
120-
if (isString(options)) {
121-
// save the value of uri so we don't have to reconstruct it later
122-
uri = options;
123-
options = url.parse(options);
135+
return request.apply(this, arguments);
124136
}
125137

126138
const span = api.createChildSpan({name: getSpanName(options)});
127139
if (!api.isRealSpan(span)) {
128-
return request(options, callback);
140+
return request.apply(this, arguments);
129141
}
130142

131-
if (!uri) {
132-
uri = extractUrl(options, protocol);
143+
if (!urlString) {
144+
urlString = extractUrl(options, protocol);
133145
}
134146

135147
const method = (options as ClientRequestArgs).method || 'GET';
136148
span.addLabel(api.labels.HTTP_METHOD_LABEL_KEY, method);
137-
span.addLabel(api.labels.HTTP_URL_LABEL_KEY, uri);
149+
span.addLabel(api.labels.HTTP_URL_LABEL_KEY, urlString);
138150

139151
// If outgoing request headers contain the "Expect" header, the returned
140152
// ClientRequest will throw an error if any new headers are added. For this
@@ -239,8 +251,8 @@ function patchHttp(http: HttpModule, api: Tracer) {
239251
// v9), so we simply follow the latter.
240252
// Ref:
241253
// https://nodejs.org/dist/latest/docs/api/http.html#http_http_get_options_callback
242-
return function getTrace(options, callback) {
243-
const req = http.request(options, callback);
254+
return function getTrace(this: never) {
255+
const req = http.request.apply(this, arguments);
244256
req.end();
245257
return req;
246258
};
@@ -255,8 +267,8 @@ function patchHttps(https: HttpsModule, api: Tracer) {
255267
return makeRequestTrace('https:', request, api);
256268
});
257269
shimmer.wrap(https, 'get', function getWrap(): typeof httpsModule.get {
258-
return function getTrace(options, callback) {
259-
const req = https.request(options, callback);
270+
return function getTrace(this: never) {
271+
const req = https.request.apply(this, arguments);
260272
req.end();
261273
return req;
262274
};

‎src/plugins/plugin-koa.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,10 @@ function createMiddleware(api: PluginTypes.Tracer): koa_1.Middleware {
121121
return function* middleware(this: koa_1.Context, next: IterableIterator<{}>) {
122122
next = startSpanForRequest(api, this, (propagateContext: boolean) => {
123123
if (propagateContext) {
124-
next.next = api.wrap(next.next);
124+
// TS Iterator definition clashes with @types/node.
125+
// For some reason, this causes the next line to not pass type check.
126+
// tslint:disable-next-line:no-any
127+
next.next = api.wrap(next.next as any);
125128
}
126129
return next;
127130
});

‎test/plugins/test-trace-http.ts‎

Lines changed: 47 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import * as httpModule from 'http';
2020
import * as httpsModule from 'https';
2121
import * as stream from 'stream';
2222
import {URL} from 'url';
23+
import * as semver from 'semver';
2324

2425
import {Constants} from '../../src/constants';
2526
import {SpanKind, TraceSpan} from '../../src/trace';
@@ -31,11 +32,11 @@ import {Express4Secure} from '../web-frameworks/express-secure';
3132

3233
// This type describes (http|https).(get|request).
3334
type HttpRequest = (
34-
options:
35-
| string
35+
url: string | httpModule.RequestOptions | httpsModule.RequestOptions | URL,
36+
options?:
3637
| httpModule.RequestOptions
3738
| httpsModule.RequestOptions
38-
| URL,
39+
| ((res: httpModule.IncomingMessage) => void),
3940
callback?: (res: httpModule.IncomingMessage) => void
4041
) => httpModule.ClientRequest;
4142

@@ -122,6 +123,25 @@ for (const nodule of Object.keys(servers) as Array<keyof typeof servers>) {
122123
await waitForResponse.done;
123124
},
124125
},
126+
{
127+
description: 'calling http.get with both url and options',
128+
fn: async () => {
129+
const waitForResponse = new WaitForResponse();
130+
http.get(
131+
'http://foo', // Fake hostname
132+
{
133+
protocol: `${nodule}:`,
134+
hostname: 'localhost',
135+
port,
136+
rejectUnauthorized: false,
137+
},
138+
waitForResponse.handleResponse
139+
);
140+
await waitForResponse.done;
141+
},
142+
// http(url, options, cb) is not a recognized signature in Node 8.
143+
versions: '>=10.x',
144+
},
125145
{
126146
description: 'calling http.get and using return value',
127147
fn: async () => {
@@ -213,22 +233,30 @@ for (const nodule of Object.keys(servers) as Array<keyof typeof servers>) {
213233
});
214234

215235
for (const testCase of testCases) {
216-
it(`creates spans with accurate timespans when ${testCase.description}`, async () => {
217-
let recordedTime = 0;
218-
await testTraceModule
219-
.get()
220-
.runInRootSpan({name: 'outer'}, async rootSpan => {
221-
assert.ok(testTraceModule.get().isRealSpan(rootSpan));
222-
recordedTime = Date.now();
223-
await testCase.fn();
224-
recordedTime = Date.now() - recordedTime;
225-
rootSpan.endSpan();
226-
});
227-
const clientSpan = testTraceModule.getOneSpan(
228-
span => span.kind === 'RPC_CLIENT'
229-
);
230-
assertSpanDuration(clientSpan, [recordedTime]);
231-
});
236+
const maybeIt =
237+
!testCase.versions ||
238+
semver.satisfies(process.version, testCase.versions)
239+
? it
240+
: it.skip;
241+
maybeIt(
242+
`creates spans with accurate timespans when ${testCase.description}`,
243+
async () => {
244+
let recordedTime = 0;
245+
await testTraceModule
246+
.get()
247+
.runInRootSpan({name: 'outer'}, async rootSpan => {
248+
assert.ok(testTraceModule.get().isRealSpan(rootSpan));
249+
recordedTime = Date.now();
250+
await testCase.fn();
251+
recordedTime = Date.now() - recordedTime;
252+
rootSpan.endSpan();
253+
});
254+
const clientSpan = testTraceModule.getOneSpan(
255+
span => span.kind === 'RPC_CLIENT'
256+
);
257+
assertSpanDuration(clientSpan, [recordedTime]);
258+
}
259+
);
232260
}
233261
});
234262

‎test/test-cls-ah.ts‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,29 +35,31 @@ maybeSkip(describe)('AsyncHooks-based CLS', () => {
3535
before(() => {
3636
asyncHooks = require('async_hooks') as AsyncHooksModule;
3737
AsyncResource = class extends asyncHooks.AsyncResource {
38+
// Polyfill for versions in which runInAsyncScope isn't defined.
39+
// This can be removed when it's guaranteed that runInAsyncScope is
40+
// present on AsyncResource.
3841
// tslint:disable:no-any
3942
runInAsyncScope<This, Result>(
4043
fn: (this: This, ...args: any[]) => Result,
4144
thisArg?: This
4245
): Result {
43-
// tslint:enable:no-any
44-
// Polyfill for versions in which runInAsyncScope isn't defined
4546
if (super.runInAsyncScope) {
4647
return super.runInAsyncScope.apply(this, arguments);
4748
} else {
4849
// tslint:disable:deprecation
49-
this.emitBefore();
50+
(this as any).emitBefore();
5051
try {
5152
return fn.apply(
5253
thisArg,
5354
Array.prototype.slice.apply(arguments).slice(2)
5455
);
5556
} finally {
56-
this.emitAfter();
57+
(this as any).emitAfter();
5758
}
5859
// tslint:enable:deprecation
5960
}
6061
}
62+
// tslint:enable:no-any
6163
};
6264
});
6365

0 commit comments

Comments
 (0)