ref(node): Partially remove dynamic require calls

packages/node/src/declarations.d.ts

@@ -1,2 +1 @@
-declare module 'https-proxy-agent';
 declare module 'async-limiter';

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);
     }
   }
 }

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');

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);

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,15 +207,20 @@ 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,
         url: 'http://9e9fd4523d784609a5fc0ebb1080592f@sentry.io:8989/mysubpath/50622',
         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;

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;