Skip to content

Commit 17f31c6

Browse files
Lms24Lms24
authored
ref(node): Partially remove dynamic require calls (#7377)
Remove a few dynamic `require` calls from our Node SDK in favor of making them top-level imports. * `Http` integration: `http` and `https` are now top-level imports. * Transport: `https-proxy-agent` is now a top-level import as v5 no longer produces side effects when importing. We need this change for the SvelteKit SDK, as SvelteKit doesn't accept `require` calls in server code by default.
1 parent a1dab3b commit 17f31c6

File tree

6 files changed

+55
-49
lines changed

6 files changed

+55
-49
lines changed

‎packages/node/src/declarations.d.ts

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,2 +1 @@
1-
declare module 'https-proxy-agent';
21
declare module 'async-limiter';

‎packages/node/src/integrations/http.ts

Lines changed: 8 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -8,8 +8,8 @@ import {
88
parseSemver,
99
stringMatchesSomePattern,
1010
} from '@sentry/utils';
11-
import type * as http from 'http';
12-
import type * as https from 'https';
11+
import * as http from 'http';
12+
import * as https from 'https';
1313
import { LRUMap } from 'lru_map';
1414

1515
import type { NodeClient } from '../client';
@@ -101,25 +101,17 @@ export class Http implements Integration {
101101
// and we will no longer have to do this optional merge, we can just pass `this._tracing` directly.
102102
const tracingOptions = this._tracing ? { ...clientOptions, ...this._tracing } : undefined;
103103

104-
// eslint-disable-next-line @typescript-eslint/no-var-requires
105-
const httpModule = require('http');
106-
const wrappedHttpHandlerMaker = _createWrappedRequestMethodFactory(this._breadcrumbs, tracingOptions, httpModule);
107-
fill(httpModule, 'get', wrappedHttpHandlerMaker);
108-
fill(httpModule, 'request', wrappedHttpHandlerMaker);
104+
const wrappedHttpHandlerMaker = _createWrappedRequestMethodFactory(this._breadcrumbs, tracingOptions, http);
105+
fill(http, 'get', wrappedHttpHandlerMaker);
106+
fill(http, 'request', wrappedHttpHandlerMaker);
109107

110108
// NOTE: Prior to Node 9, `https` used internals of `http` module, thus we don't patch it.
111109
// If we do, we'd get double breadcrumbs and double spans for `https` calls.
112110
// It has been changed in Node 9, so for all versions equal and above, we patch `https` separately.
113111
if (NODE_VERSION.major && NODE_VERSION.major > 8) {
114-
// eslint-disable-next-line @typescript-eslint/no-var-requires
115-
const httpsModule = require('https');
116-
const wrappedHttpsHandlerMaker = _createWrappedRequestMethodFactory(
117-
this._breadcrumbs,
118-
tracingOptions,
119-
httpsModule,
120-
);
121-
fill(httpsModule, 'get', wrappedHttpsHandlerMaker);
122-
fill(httpsModule, 'request', wrappedHttpsHandlerMaker);
112+
const wrappedHttpsHandlerMaker = _createWrappedRequestMethodFactory(this._breadcrumbs, tracingOptions, https);
113+
fill(https, 'get', wrappedHttpsHandlerMaker);
114+
fill(https, 'request', wrappedHttpsHandlerMaker);
123115
}
124116
}
125117
}

‎packages/node/src/integrations/localvariables.ts

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,19 @@ class AsyncSession implements DebugSession {
2828

2929
/** Throws is inspector API is not available */
3030
public constructor() {
31+
/*
32+
TODO: We really should get rid of this require statement below for a couple of reasons:
33+
1. It makes the integration unusable in the SvelteKit SDK, as it's not possible to use `require`
34+
in SvelteKit server code (at least not by default).
35+
2. Throwing in a constructor is bad practice
36+
37+
More context for a future attempt to fix this:
38+
We already tried replacing it with import but didn't get it to work because of async problems.
39+
We still called import in the constructor but assigned to a promise which we "awaited" in
40+
`configureAndConnect`. However, this broke the Node integration tests as no local variables
41+
were reported any more. We probably missed a place where we need to await the promise, too.
42+
*/
43+
3144
// Node can be build without inspector support so this can throw
3245
// eslint-disable-next-line @typescript-eslint/no-var-requires
3346
const { Session } = require('inspector');

‎packages/node/src/transports/http.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import type {
88
} from '@sentry/types';
99
import * as http from 'http';
1010
import * as https from 'https';
11+
import { HttpsProxyAgent } from 'https-proxy-agent';
1112
import { Readable } from 'stream';
1213
import { URL } from 'url';
1314
import { createGzip } from 'zlib';
@@ -74,8 +75,7 @@ export function makeNodeTransport(options: NodeTransportOptions): Transport {
7475
// TODO(v7): Evaluate if we can set keepAlive to true. This would involve testing for memory leaks in older node
7576
// versions(>= 8) as they had memory leaks when using it: #2555
7677
const agent = proxy
77-
? // eslint-disable-next-line @typescript-eslint/no-var-requires
78-
(new (require('https-proxy-agent'))(proxy) as http.Agent)
78+
? (new HttpsProxyAgent(proxy) as http.Agent)
7979
: new nativeHttpModule.Agent({ keepAlive, maxSockets: 30, timeout: 2000 });
8080

8181
const requestExecutor = createRequestExecutor(options, options.httpModule ?? nativeHttpModule, agent);

‎packages/node/test/transports/http.test.ts

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,7 @@ jest.mock('@sentry/core', () => {
1717
};
1818
});
1919

20-
// eslint-disable-next-line @typescript-eslint/no-var-requires
21-
const httpProxyAgent = require('https-proxy-agent');
22-
jest.mock('https-proxy-agent', () => {
23-
return jest.fn().mockImplementation(() => new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 }));
24-
});
20+
import * as httpProxyAgent from 'https-proxy-agent';
2521

2622
const SUCCESS = 200;
2723
const RATE_LIMIT = 429;
@@ -211,15 +207,20 @@ describe('makeNewHttpTransport()', () => {
211207
});
212208

213209
describe('proxy', () => {
210+
const proxyAgentSpy = jest
211+
.spyOn(httpProxyAgent, 'HttpsProxyAgent')
212+
// @ts-ignore
213+
.mockImplementation(() => new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 }));
214+
214215
it('can be configured through option', () => {
215216
makeNodeTransport({
216217
...defaultOptions,
217218
url: 'http://[email protected]:8989/mysubpath/50622',
218219
proxy: 'http://example.com',
219220
});
220221

221-
expect(httpProxyAgent).toHaveBeenCalledTimes(1);
222-
expect(httpProxyAgent).toHaveBeenCalledWith('http://example.com');
222+
expect(proxyAgentSpy).toHaveBeenCalledTimes(1);
223+
expect(proxyAgentSpy).toHaveBeenCalledWith('http://example.com');
223224
});
224225

225226
it('can be configured through env variables option', () => {
@@ -229,8 +230,8 @@ describe('makeNewHttpTransport()', () => {
229230
url: 'http://[email protected]:8989/mysubpath/50622',
230231
});
231232

232-
expect(httpProxyAgent).toHaveBeenCalledTimes(1);
233-
expect(httpProxyAgent).toHaveBeenCalledWith('http://example.com');
233+
expect(proxyAgentSpy).toHaveBeenCalledTimes(1);
234+
expect(proxyAgentSpy).toHaveBeenCalledWith('http://example.com');
234235
delete process.env.http_proxy;
235236
});
236237

@@ -242,8 +243,8 @@ describe('makeNewHttpTransport()', () => {
242243
proxy: 'http://bar.com',
243244
});
244245

245-
expect(httpProxyAgent).toHaveBeenCalledTimes(1);
246-
expect(httpProxyAgent).toHaveBeenCalledWith('http://bar.com');
246+
expect(proxyAgentSpy).toHaveBeenCalledTimes(1);
247+
expect(proxyAgentSpy).toHaveBeenCalledWith('http://bar.com');
247248
delete process.env.http_proxy;
248249
});
249250

@@ -255,7 +256,7 @@ describe('makeNewHttpTransport()', () => {
255256
proxy: 'http://example.com',
256257
});
257258

258-
expect(httpProxyAgent).not.toHaveBeenCalled();
259+
expect(proxyAgentSpy).not.toHaveBeenCalled();
259260

260261
delete process.env.no_proxy;
261262
});
@@ -269,7 +270,7 @@ describe('makeNewHttpTransport()', () => {
269270
url: 'http://[email protected]:8989/mysubpath/50622',
270271
});
271272

272-
expect(httpProxyAgent).not.toHaveBeenCalled();
273+
expect(proxyAgentSpy).not.toHaveBeenCalled();
273274

274275
delete process.env.no_proxy;
275276
delete process.env.http_proxy;
@@ -284,7 +285,7 @@ describe('makeNewHttpTransport()', () => {
284285
url: 'http://[email protected]:8989/mysubpath/50622',
285286
});
286287

287-
expect(httpProxyAgent).not.toHaveBeenCalled();
288+
expect(proxyAgentSpy).not.toHaveBeenCalled();
288289

289290
delete process.env.no_proxy;
290291
delete process.env.http_proxy;

‎packages/node/test/transports/https.test.ts

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,7 @@ jest.mock('@sentry/core', () => {
1919
};
2020
});
2121

22-
// eslint-disable-next-line @typescript-eslint/no-var-requires
23-
const httpProxyAgent = require('https-proxy-agent');
24-
jest.mock('https-proxy-agent', () => {
25-
return jest.fn().mockImplementation(() => new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 }));
26-
});
22+
import * as httpProxyAgent from 'https-proxy-agent';
2723

2824
const SUCCESS = 200;
2925
const RATE_LIMIT = 429;
@@ -185,6 +181,11 @@ describe('makeNewHttpsTransport()', () => {
185181
});
186182

187183
describe('proxy', () => {
184+
const proxyAgentSpy = jest
185+
.spyOn(httpProxyAgent, 'HttpsProxyAgent')
186+
// @ts-ignore
187+
.mockImplementation(() => new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 }));
188+
188189
it('can be configured through option', () => {
189190
makeNodeTransport({
190191
...defaultOptions,
@@ -193,8 +194,8 @@ describe('makeNewHttpsTransport()', () => {
193194
proxy: 'https://example.com',
194195
});
195196

196-
expect(httpProxyAgent).toHaveBeenCalledTimes(1);
197-
expect(httpProxyAgent).toHaveBeenCalledWith('https://example.com');
197+
expect(proxyAgentSpy).toHaveBeenCalledTimes(1);
198+
expect(proxyAgentSpy).toHaveBeenCalledWith('https://example.com');
198199
});
199200

200201
it('can be configured through env variables option (http)', () => {
@@ -205,8 +206,8 @@ describe('makeNewHttpsTransport()', () => {
205206
url: 'https://[email protected]:8989/mysubpath/50622',
206207
});
207208

208-
expect(httpProxyAgent).toHaveBeenCalledTimes(1);
209-
expect(httpProxyAgent).toHaveBeenCalledWith('https://example.com');
209+
expect(proxyAgentSpy).toHaveBeenCalledTimes(1);
210+
expect(proxyAgentSpy).toHaveBeenCalledWith('https://example.com');
210211
delete process.env.http_proxy;
211212
});
212213

@@ -218,8 +219,8 @@ describe('makeNewHttpsTransport()', () => {
218219
url: 'https://[email protected]:8989/mysubpath/50622',
219220
});
220221

221-
expect(httpProxyAgent).toHaveBeenCalledTimes(1);
222-
expect(httpProxyAgent).toHaveBeenCalledWith('https://example.com');
222+
expect(proxyAgentSpy).toHaveBeenCalledTimes(1);
223+
expect(proxyAgentSpy).toHaveBeenCalledWith('https://example.com');
223224
delete process.env.https_proxy;
224225
});
225226

@@ -232,8 +233,8 @@ describe('makeNewHttpsTransport()', () => {
232233
proxy: 'https://bar.com',
233234
});
234235

235-
expect(httpProxyAgent).toHaveBeenCalledTimes(1);
236-
expect(httpProxyAgent).toHaveBeenCalledWith('https://bar.com');
236+
expect(proxyAgentSpy).toHaveBeenCalledTimes(1);
237+
expect(proxyAgentSpy).toHaveBeenCalledWith('https://bar.com');
237238
delete process.env.https_proxy;
238239
});
239240

@@ -246,7 +247,7 @@ describe('makeNewHttpsTransport()', () => {
246247
proxy: 'https://example.com',
247248
});
248249

249-
expect(httpProxyAgent).not.toHaveBeenCalled();
250+
expect(proxyAgentSpy).not.toHaveBeenCalled();
250251

251252
delete process.env.no_proxy;
252253
});
@@ -261,7 +262,7 @@ describe('makeNewHttpsTransport()', () => {
261262
url: 'https://[email protected]:8989/mysubpath/50622',
262263
});
263264

264-
expect(httpProxyAgent).not.toHaveBeenCalled();
265+
expect(proxyAgentSpy).not.toHaveBeenCalled();
265266

266267
delete process.env.no_proxy;
267268
delete process.env.http_proxy;
@@ -277,7 +278,7 @@ describe('makeNewHttpsTransport()', () => {
277278
url: 'https://[email protected]:8989/mysubpath/50622',
278279
});
279280

280-
expect(httpProxyAgent).not.toHaveBeenCalled();
281+
expect(proxyAgentSpy).not.toHaveBeenCalled();
281282

282283
delete process.env.no_proxy;
283284
delete process.env.http_proxy;

0 commit comments

Comments
 (0)