diff --git a/packages/node/src/declarations.d.ts b/packages/node/src/declarations.d.ts index 7d9d77b82bfb..843b79454518 100644 --- a/packages/node/src/declarations.d.ts +++ b/packages/node/src/declarations.d.ts @@ -1,2 +1 @@ -declare module 'https-proxy-agent'; declare module 'async-limiter'; diff --git a/packages/node/src/integrations/http.ts b/packages/node/src/integrations/http.ts index ea7a9eae173d..017929fbe0eb 100644 --- a/packages/node/src/integrations/http.ts +++ b/packages/node/src/integrations/http.ts @@ -8,8 +8,8 @@ import { parseSemver, stringMatchesSomePattern, } from '@sentry/utils'; -import type * as http from 'http'; -import type * as https from 'https'; +import * as http from 'http'; +import * as https from 'https'; import { LRUMap } from 'lru_map'; import type { NodeClient } from '../client'; @@ -101,25 +101,17 @@ export class Http implements Integration { // and we will no longer have to do this optional merge, we can just pass `this._tracing` directly. const tracingOptions = this._tracing ? { ...clientOptions, ...this._tracing } : undefined; - // eslint-disable-next-line @typescript-eslint/no-var-requires - const httpModule = require('http'); - const wrappedHttpHandlerMaker = _createWrappedRequestMethodFactory(this._breadcrumbs, tracingOptions, httpModule); - fill(httpModule, 'get', wrappedHttpHandlerMaker); - fill(httpModule, 'request', wrappedHttpHandlerMaker); + const wrappedHttpHandlerMaker = _createWrappedRequestMethodFactory(this._breadcrumbs, tracingOptions, http); + fill(http, 'get', wrappedHttpHandlerMaker); + fill(http, 'request', wrappedHttpHandlerMaker); // NOTE: Prior to Node 9, `https` used internals of `http` module, thus we don't patch it. // If we do, we'd get double breadcrumbs and double spans for `https` calls. // It has been changed in Node 9, so for all versions equal and above, we patch `https` separately. if (NODE_VERSION.major && NODE_VERSION.major > 8) { - // eslint-disable-next-line @typescript-eslint/no-var-requires - const httpsModule = require('https'); - const wrappedHttpsHandlerMaker = _createWrappedRequestMethodFactory( - this._breadcrumbs, - tracingOptions, - httpsModule, - ); - fill(httpsModule, 'get', wrappedHttpsHandlerMaker); - fill(httpsModule, 'request', wrappedHttpsHandlerMaker); + const wrappedHttpsHandlerMaker = _createWrappedRequestMethodFactory(this._breadcrumbs, tracingOptions, https); + fill(https, 'get', wrappedHttpsHandlerMaker); + fill(https, 'request', wrappedHttpsHandlerMaker); } } } diff --git a/packages/node/src/integrations/localvariables.ts b/packages/node/src/integrations/localvariables.ts index 2423cfa30c70..648a9e88d3be 100644 --- a/packages/node/src/integrations/localvariables.ts +++ b/packages/node/src/integrations/localvariables.ts @@ -28,6 +28,19 @@ class AsyncSession implements DebugSession { /** Throws is inspector API is not available */ public constructor() { + /* + TODO: We really should get rid of this require statement below for a couple of reasons: + 1. It makes the integration unusable in the SvelteKit SDK, as it's not possible to use `require` + in SvelteKit server code (at least not by default). + 2. Throwing in a constructor is bad practice + + More context for a future attempt to fix this: + We already tried replacing it with import but didn't get it to work because of async problems. + We still called import in the constructor but assigned to a promise which we "awaited" in + `configureAndConnect`. However, this broke the Node integration tests as no local variables + were reported any more. We probably missed a place where we need to await the promise, too. + */ + // Node can be build without inspector support so this can throw // eslint-disable-next-line @typescript-eslint/no-var-requires const { Session } = require('inspector'); diff --git a/packages/node/src/transports/http.ts b/packages/node/src/transports/http.ts index a16f78e79a6f..340bcf4800f0 100644 --- a/packages/node/src/transports/http.ts +++ b/packages/node/src/transports/http.ts @@ -8,6 +8,7 @@ import type { } from '@sentry/types'; import * as http from 'http'; import * as https from 'https'; +import { HttpsProxyAgent } from 'https-proxy-agent'; import { Readable } from 'stream'; import { URL } from 'url'; import { createGzip } from 'zlib'; @@ -74,8 +75,7 @@ export function makeNodeTransport(options: NodeTransportOptions): Transport { // TODO(v7): Evaluate if we can set keepAlive to true. This would involve testing for memory leaks in older node // versions(>= 8) as they had memory leaks when using it: #2555 const agent = proxy - ? // eslint-disable-next-line @typescript-eslint/no-var-requires - (new (require('https-proxy-agent'))(proxy) as http.Agent) + ? (new HttpsProxyAgent(proxy) as http.Agent) : new nativeHttpModule.Agent({ keepAlive, maxSockets: 30, timeout: 2000 }); const requestExecutor = createRequestExecutor(options, options.httpModule ?? nativeHttpModule, agent); diff --git a/packages/node/test/transports/http.test.ts b/packages/node/test/transports/http.test.ts index d03e917c4f42..58b2710f1ac5 100644 --- a/packages/node/test/transports/http.test.ts +++ b/packages/node/test/transports/http.test.ts @@ -17,11 +17,7 @@ jest.mock('@sentry/core', () => { }; }); -// eslint-disable-next-line @typescript-eslint/no-var-requires -const httpProxyAgent = require('https-proxy-agent'); -jest.mock('https-proxy-agent', () => { - return jest.fn().mockImplementation(() => new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 })); -}); +import * as httpProxyAgent from 'https-proxy-agent'; const SUCCESS = 200; const RATE_LIMIT = 429; @@ -211,6 +207,11 @@ describe('makeNewHttpTransport()', () => { }); describe('proxy', () => { + const proxyAgentSpy = jest + .spyOn(httpProxyAgent, 'HttpsProxyAgent') + // @ts-ignore + .mockImplementation(() => new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 })); + it('can be configured through option', () => { makeNodeTransport({ ...defaultOptions, @@ -218,8 +219,8 @@ describe('makeNewHttpTransport()', () => { proxy: 'http://example.com', }); - expect(httpProxyAgent).toHaveBeenCalledTimes(1); - expect(httpProxyAgent).toHaveBeenCalledWith('http://example.com'); + expect(proxyAgentSpy).toHaveBeenCalledTimes(1); + expect(proxyAgentSpy).toHaveBeenCalledWith('http://example.com'); }); it('can be configured through env variables option', () => { @@ -229,8 +230,8 @@ describe('makeNewHttpTransport()', () => { url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); - expect(httpProxyAgent).toHaveBeenCalledTimes(1); - expect(httpProxyAgent).toHaveBeenCalledWith('http://example.com'); + expect(proxyAgentSpy).toHaveBeenCalledTimes(1); + expect(proxyAgentSpy).toHaveBeenCalledWith('http://example.com'); delete process.env.http_proxy; }); @@ -242,8 +243,8 @@ describe('makeNewHttpTransport()', () => { proxy: 'http://bar.com', }); - expect(httpProxyAgent).toHaveBeenCalledTimes(1); - expect(httpProxyAgent).toHaveBeenCalledWith('http://bar.com'); + expect(proxyAgentSpy).toHaveBeenCalledTimes(1); + expect(proxyAgentSpy).toHaveBeenCalledWith('http://bar.com'); delete process.env.http_proxy; }); @@ -255,7 +256,7 @@ describe('makeNewHttpTransport()', () => { proxy: 'http://example.com', }); - expect(httpProxyAgent).not.toHaveBeenCalled(); + expect(proxyAgentSpy).not.toHaveBeenCalled(); delete process.env.no_proxy; }); @@ -269,7 +270,7 @@ describe('makeNewHttpTransport()', () => { url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); - expect(httpProxyAgent).not.toHaveBeenCalled(); + expect(proxyAgentSpy).not.toHaveBeenCalled(); delete process.env.no_proxy; delete process.env.http_proxy; @@ -284,7 +285,7 @@ describe('makeNewHttpTransport()', () => { url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); - expect(httpProxyAgent).not.toHaveBeenCalled(); + expect(proxyAgentSpy).not.toHaveBeenCalled(); delete process.env.no_proxy; delete process.env.http_proxy; diff --git a/packages/node/test/transports/https.test.ts b/packages/node/test/transports/https.test.ts index d43b8306bed0..e63898a3b11e 100644 --- a/packages/node/test/transports/https.test.ts +++ b/packages/node/test/transports/https.test.ts @@ -19,11 +19,7 @@ jest.mock('@sentry/core', () => { }; }); -// eslint-disable-next-line @typescript-eslint/no-var-requires -const httpProxyAgent = require('https-proxy-agent'); -jest.mock('https-proxy-agent', () => { - return jest.fn().mockImplementation(() => new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 })); -}); +import * as httpProxyAgent from 'https-proxy-agent'; const SUCCESS = 200; const RATE_LIMIT = 429; @@ -185,6 +181,11 @@ describe('makeNewHttpsTransport()', () => { }); describe('proxy', () => { + const proxyAgentSpy = jest + .spyOn(httpProxyAgent, 'HttpsProxyAgent') + // @ts-ignore + .mockImplementation(() => new http.Agent({ keepAlive: false, maxSockets: 30, timeout: 2000 })); + it('can be configured through option', () => { makeNodeTransport({ ...defaultOptions, @@ -193,8 +194,8 @@ describe('makeNewHttpsTransport()', () => { proxy: 'https://example.com', }); - expect(httpProxyAgent).toHaveBeenCalledTimes(1); - expect(httpProxyAgent).toHaveBeenCalledWith('https://example.com'); + expect(proxyAgentSpy).toHaveBeenCalledTimes(1); + expect(proxyAgentSpy).toHaveBeenCalledWith('https://example.com'); }); it('can be configured through env variables option (http)', () => { @@ -205,8 +206,8 @@ describe('makeNewHttpsTransport()', () => { url: 'https://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); - expect(httpProxyAgent).toHaveBeenCalledTimes(1); - expect(httpProxyAgent).toHaveBeenCalledWith('https://example.com'); + expect(proxyAgentSpy).toHaveBeenCalledTimes(1); + expect(proxyAgentSpy).toHaveBeenCalledWith('https://example.com'); delete process.env.http_proxy; }); @@ -218,8 +219,8 @@ describe('makeNewHttpsTransport()', () => { url: 'https://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); - expect(httpProxyAgent).toHaveBeenCalledTimes(1); - expect(httpProxyAgent).toHaveBeenCalledWith('https://example.com'); + expect(proxyAgentSpy).toHaveBeenCalledTimes(1); + expect(proxyAgentSpy).toHaveBeenCalledWith('https://example.com'); delete process.env.https_proxy; }); @@ -232,8 +233,8 @@ describe('makeNewHttpsTransport()', () => { proxy: 'https://bar.com', }); - expect(httpProxyAgent).toHaveBeenCalledTimes(1); - expect(httpProxyAgent).toHaveBeenCalledWith('https://bar.com'); + expect(proxyAgentSpy).toHaveBeenCalledTimes(1); + expect(proxyAgentSpy).toHaveBeenCalledWith('https://bar.com'); delete process.env.https_proxy; }); @@ -246,7 +247,7 @@ describe('makeNewHttpsTransport()', () => { proxy: 'https://example.com', }); - expect(httpProxyAgent).not.toHaveBeenCalled(); + expect(proxyAgentSpy).not.toHaveBeenCalled(); delete process.env.no_proxy; }); @@ -261,7 +262,7 @@ describe('makeNewHttpsTransport()', () => { url: 'https://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); - expect(httpProxyAgent).not.toHaveBeenCalled(); + expect(proxyAgentSpy).not.toHaveBeenCalled(); delete process.env.no_proxy; delete process.env.http_proxy; @@ -277,7 +278,7 @@ describe('makeNewHttpsTransport()', () => { url: 'https://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622', }); - expect(httpProxyAgent).not.toHaveBeenCalled(); + expect(proxyAgentSpy).not.toHaveBeenCalled(); delete process.env.no_proxy; delete process.env.http_proxy;