From 29b74727c0e744086ee76364ea9d48c7c5fc4c94 Mon Sep 17 00:00:00 2001 From: isaacs Date: Fri, 10 Apr 2026 08:31:14 -0700 Subject: [PATCH 1/2] fix(core, node): support loading Express options lazily Update the Express integration to accept the module export and a configuration function, rather than a configuration object. This is needed to support lazily calling Sentry.init *after* the module has been instrumented, without re-wrapping the methods to get the new config. via: @mydea in #20188 --- .../suites/express/late-init/instrument.mjs | 19 +++++++ .../suites/express/late-init/scenario.mjs | 18 ++++++ .../suites/express/late-init/test.ts | 49 ++++++++++++++++ .../core/src/integrations/express/index.ts | 13 +++-- .../src/integrations/express/patch-layer.ts | 8 ++- .../core/src/integrations/express/types.ts | 1 - .../lib/integrations/express/index.test.ts | 57 +++++++++---------- .../integrations/express/patch-layer.test.ts | 30 +++++----- .../node/src/integrations/tracing/express.ts | 10 ++-- 9 files changed, 147 insertions(+), 58 deletions(-) create mode 100644 dev-packages/node-integration-tests/suites/express/late-init/instrument.mjs create mode 100644 dev-packages/node-integration-tests/suites/express/late-init/scenario.mjs create mode 100644 dev-packages/node-integration-tests/suites/express/late-init/test.ts diff --git a/dev-packages/node-integration-tests/suites/express/late-init/instrument.mjs b/dev-packages/node-integration-tests/suites/express/late-init/instrument.mjs new file mode 100644 index 000000000000..eea577d83ebc --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/late-init/instrument.mjs @@ -0,0 +1,19 @@ +import * as Sentry from '@sentry/node'; +import { loggingTransport } from '@sentry-internal/node-integration-tests'; + +// First: preload the express instrumentation without calling Sentry.init(). +// registers OTel module hook, patches the Express module with no config. +Sentry.preloadOpenTelemetry({ integrations: ['Express'] }); + +// call Sentry.init() with express integration config. +// instrumentExpress is already registered, so this calls setConfig() on the +// existing instrumentation to update its options. The lazy getOptions() +// in patchLayer ensures the updated options are read at request time. +Sentry.init({ + dsn: 'https://public@dsn.ingest.sentry.io/1337', + release: '1.0', + tracesSampleRate: 1.0, + transport: loggingTransport, + // suppress the middleware layer that the cors module generates + integrations: [Sentry.expressIntegration({ ignoreLayersType: ['middleware'] })], +}); diff --git a/dev-packages/node-integration-tests/suites/express/late-init/scenario.mjs b/dev-packages/node-integration-tests/suites/express/late-init/scenario.mjs new file mode 100644 index 000000000000..faea295143ef --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/late-init/scenario.mjs @@ -0,0 +1,18 @@ +import cors from 'cors'; +import express from 'express'; +import * as Sentry from '@sentry/node'; +import { startExpressServerAndSendPortToRunner } from '@sentry-internal/node-integration-tests'; + +const app = express(); + +// cors() would normally create a 'middleware' type span, but the +// ignoreLayersType: ['middleware'] option set via Sentry.init() suppresses it. +app.use(cors()); + +app.get('/test/express', (_req, res) => { + res.send({ response: 'response 1' }); +}); + +Sentry.setupExpressErrorHandler(app); + +startExpressServerAndSendPortToRunner(app); diff --git a/dev-packages/node-integration-tests/suites/express/late-init/test.ts b/dev-packages/node-integration-tests/suites/express/late-init/test.ts new file mode 100644 index 000000000000..d7aeba94fd2b --- /dev/null +++ b/dev-packages/node-integration-tests/suites/express/late-init/test.ts @@ -0,0 +1,49 @@ +import { afterAll, describe, expect } from 'vitest'; +import { assertSentryTransaction } from '../../../utils/assertions'; +import { cleanupChildProcesses, createEsmAndCjsTests } from '../../../utils/runner'; + +describe('express late init', () => { + afterAll(() => { + cleanupChildProcesses(); + }); + + createEsmAndCjsTests(__dirname, 'scenario.mjs', 'instrument.mjs', (createRunner, test) => { + test('applies expressIntegration config set via Sentry.init() called after instrumentExpress()', async () => { + const runner = createRunner() + .expect({ + transaction: transaction => { + assertSentryTransaction(transaction, { + transaction: 'GET /test/express', + contexts: { + trace: { + op: 'http.server', + status: 'ok', + }, + }, + }); + // request_handler span IS present + // confirms the express patch was applied. + expect(transaction.spans).toContainEqual( + expect.objectContaining({ + data: expect.objectContaining({ + 'express.type': 'request_handler', + }), + }), + ); + // Middleware spans NOT present, ignoreLayersType: ['middleware'] + // configured via the Sentry.init() AFTER instrumentExpress(). + expect(transaction.spans).not.toContainEqual( + expect.objectContaining({ + data: expect.objectContaining({ + 'express.type': 'middleware', + }), + }), + ); + }, + }) + .start(); + runner.makeRequest('get', '/test/express'); + await runner.completed(); + }); + }); +}); diff --git a/packages/core/src/integrations/express/index.ts b/packages/core/src/integrations/express/index.ts index 4b2d5e2f0677..f1dd8a301c17 100644 --- a/packages/core/src/integrations/express/index.ts +++ b/packages/core/src/integrations/express/index.ts @@ -69,11 +69,12 @@ const getExpressExport = (express: ExpressModuleExport): ExpressExport => * import express from 'express'; * import * as Sentry from '@sentry/deno'; // or any SDK that extends core * - * Sentry.patchExpressModule({ express }) + * Sentry.patchExpressModule(express, () => ({})); + * ``` */ -export const patchExpressModule = (options: ExpressIntegrationOptions) => { +export const patchExpressModule = (moduleExports: ExpressModuleExport, getOptions: () => ExpressIntegrationOptions) => { // pass in the require() or import() result of express - const express = getExpressExport(options.express); + const express = getExpressExport(moduleExports); const routerProto: ExpressRouterv4 | ExpressRouterv5 | undefined = isExpressWithRouterPrototype(express) ? express.Router.prototype // Express v5 : isExpressWithoutRouterPrototype(express) @@ -93,7 +94,7 @@ export const patchExpressModule = (options: ExpressIntegrationOptions) => { function routeTrace(this: ExpressRouter, ...args: Parameters[]) { const route = originalRouteMethod.apply(this, args); const layer = this.stack[this.stack.length - 1] as ExpressLayer; - patchLayer(options, layer, getLayerPath(args)); + patchLayer(getOptions, layer, getLayerPath(args)); return route; }, ); @@ -113,7 +114,7 @@ export const patchExpressModule = (options: ExpressIntegrationOptions) => { if (!layer) { return route; } - patchLayer(options, layer, getLayerPath(args)); + patchLayer(getOptions, layer, getLayerPath(args)); return route; }, ); @@ -141,7 +142,7 @@ export const patchExpressModule = (options: ExpressIntegrationOptions) => { if (router) { const layer = router.stack[router.stack.length - 1]; if (layer) { - patchLayer(options, layer, getLayerPath(args)); + patchLayer(getOptions, layer, getLayerPath(args)); } } return route; diff --git a/packages/core/src/integrations/express/patch-layer.ts b/packages/core/src/integrations/express/patch-layer.ts index 5f537e403524..3114cbafb320 100644 --- a/packages/core/src/integrations/express/patch-layer.ts +++ b/packages/core/src/integrations/express/patch-layer.ts @@ -61,7 +61,11 @@ export type ExpressPatchLayerOptions = Pick< 'onRouteResolved' | 'ignoreLayers' | 'ignoreLayersType' >; -export function patchLayer(options: ExpressPatchLayerOptions, maybeLayer?: ExpressLayer, layerPath?: string): void { +export function patchLayer( + getOptions: () => ExpressPatchLayerOptions, + maybeLayer?: ExpressLayer, + layerPath?: string, +): void { if (!maybeLayer?.handle) { return; } @@ -86,6 +90,8 @@ export function patchLayer(options: ExpressPatchLayerOptions, maybeLayer?: Expre //oxlint-disable-next-line no-explicit-any ...otherArgs: any[] ) { + const options = getOptions(); + // Set normalizedRequest here because expressRequestHandler middleware // (registered via setupExpressErrorHandler) is added after routes and // therefore never runs for successful requests — route handlers typically diff --git a/packages/core/src/integrations/express/types.ts b/packages/core/src/integrations/express/types.ts index 66d6f1de3c9e..406b2d460e3f 100644 --- a/packages/core/src/integrations/express/types.ts +++ b/packages/core/src/integrations/express/types.ts @@ -136,7 +136,6 @@ export type ExpressRouter = { export type IgnoreMatcher = string | RegExp | ((name: string) => boolean); export type ExpressIntegrationOptions = { - express: ExpressModuleExport; //Express /** Ignore specific based on their name */ ignoreLayers?: IgnoreMatcher[]; /** Ignore specific layers based on their type */ diff --git a/packages/core/test/lib/integrations/express/index.test.ts b/packages/core/test/lib/integrations/express/index.test.ts index 55fe442efb22..9e464b1b0adf 100644 --- a/packages/core/test/lib/integrations/express/index.test.ts +++ b/packages/core/test/lib/integrations/express/index.test.ts @@ -61,12 +61,12 @@ vi.mock('../../../../src/utils/debug-logger', () => ({ })); beforeEach(() => (patchLayerCalls.length = 0)); -const patchLayerCalls: [options: ExpressIntegrationOptions, layer: ExpressLayer, layerPath?: string][] = []; +const patchLayerCalls: [getOptions: () => ExpressIntegrationOptions, layer: ExpressLayer, layerPath?: string][] = []; vi.mock('../../../../src/integrations/express/patch-layer', () => ({ - patchLayer: (options: ExpressIntegrationOptions, layer?: ExpressLayer, layerPath?: string) => { + patchLayer: (getOptions: () => ExpressIntegrationOptions, layer?: ExpressLayer, layerPath?: string) => { if (layer) { - patchLayerCalls.push([options, layer, layerPath]); + patchLayerCalls.push([getOptions, layer, layerPath]); } }, })); @@ -131,24 +131,22 @@ function getExpress5(): ExpressExportv5 & { spies: ExpressSpies } { describe('patchExpressModule', () => { it('throws trying to patch/unpatch the wrong thing', () => { expect(() => { - patchExpressModule({ - express: {} as unknown as ExpressModuleExport, - } as unknown as ExpressIntegrationOptions); + patchExpressModule({} as unknown as ExpressModuleExport, () => ({})); }).toThrowError('no valid Express route function to instrument'); }); it('can patch and restore expressv4 style module', () => { for (const useDefault of [false, true]) { const express = getExpress4(); - const module = useDefault ? { default: express } : express; + const moduleExports = useDefault ? { default: express } : express; const r = express.Router as ExpressRouterv4; const a = express.application; - const options = { express: module } as unknown as ExpressIntegrationOptions; + const getOptions = () => ({}); expect((r.use as WrappedFunction).__sentry_original__).toBe(undefined); expect((r.route as WrappedFunction).__sentry_original__).toBe(undefined); expect((a.use as WrappedFunction).__sentry_original__).toBe(undefined); - patchExpressModule(options); + patchExpressModule(moduleExports, getOptions); expect(typeof (r.use as WrappedFunction).__sentry_original__).toBe('function'); expect(typeof (r.route as WrappedFunction).__sentry_original__).toBe('function'); @@ -161,13 +159,13 @@ describe('patchExpressModule', () => { const express = getExpress5(); const r = express.Router as ExpressRouterv5; const a = express.application; - const module = useDefault ? { default: express } : express; - const options = { express: module } as unknown as ExpressIntegrationOptions; + const moduleExports = useDefault ? { default: express } : express; + const getOptions = () => ({}); expect((r.prototype.use as WrappedFunction).__sentry_original__).toBe(undefined); expect((r.prototype.route as WrappedFunction).__sentry_original__).toBe(undefined); expect((a.use as WrappedFunction).__sentry_original__).toBe(undefined); - patchExpressModule(options); + patchExpressModule(moduleExports, getOptions); expect(typeof (r.prototype.use as WrappedFunction).__sentry_original__).toBe('function'); expect(typeof (r.prototype.route as WrappedFunction).__sentry_original__).toBe('function'); @@ -178,8 +176,8 @@ describe('patchExpressModule', () => { it('calls patched and original Router.route', () => { const expressv4 = getExpress4(); const { spies } = expressv4; - const options = { express: expressv4 }; - patchExpressModule(options); + const getOptions = () => ({}); + patchExpressModule(expressv4, getOptions); expressv4.Router.route('a'); expect(spies.routerRoute).toHaveBeenCalledExactlyOnceWith('a'); }); @@ -187,18 +185,18 @@ describe('patchExpressModule', () => { it('calls patched and original Router.use', () => { const expressv4 = getExpress4(); const { spies } = expressv4; - const options = { express: expressv4 }; - patchExpressModule(options); + const getOptions = () => ({}); + patchExpressModule(expressv4, getOptions); expressv4.Router.use('a'); - expect(patchLayerCalls).toStrictEqual([[options, { name: 'layerFinal' }, 'a']]); + expect(patchLayerCalls).toStrictEqual([[getOptions, { name: 'layerFinal' }, 'a']]); expect(spies.routerUse).toHaveBeenCalledExactlyOnceWith('a'); }); it('skips patchLayer call in Router.use if no layer in the stack', () => { const expressv4 = getExpress4(); const { spies } = expressv4; - const options = { express: expressv4 }; - patchExpressModule(options); + const getOptions = () => ({}); + patchExpressModule(expressv4, getOptions); const { stack } = expressv4.Router; expressv4.Router.stack = []; expressv4.Router.use('a'); @@ -210,28 +208,28 @@ describe('patchExpressModule', () => { it('calls patched and original application.use', () => { const expressv4 = getExpress4(); const { spies } = expressv4; - const options = { express: expressv4 }; - patchExpressModule(options); + const getOptions = () => ({}); + patchExpressModule(expressv4, getOptions); expressv4.application.use('a'); - expect(patchLayerCalls).toStrictEqual([[options, { name: 'layerFinal' }, 'a']]); + expect(patchLayerCalls).toStrictEqual([[getOptions, { name: 'layerFinal' }, 'a']]); expect(spies.appUse).toHaveBeenCalledExactlyOnceWith('a'); }); it('calls patched and original application.use on express v5', () => { const expressv5 = getExpress5(); const { spies } = expressv5; - const options = { express: expressv5 }; - patchExpressModule(options); + const getOptions = () => ({}); + patchExpressModule(expressv5, getOptions); expressv5.application.use('a'); - expect(patchLayerCalls).toStrictEqual([[options, { name: 'layerFinal' }, 'a']]); + expect(patchLayerCalls).toStrictEqual([[getOptions, { name: 'layerFinal' }, 'a']]); expect(spies.appUse).toHaveBeenCalledExactlyOnceWith('a'); }); it('skips patchLayer on application.use if no router found', () => { const expressv4 = getExpress4(); const { spies } = expressv4; - const options = { express: expressv4 }; - patchExpressModule(options); + const getOptions = () => ({}); + patchExpressModule(expressv4, getOptions); const app = expressv4.application as { _router?: ExpressRoute; }; @@ -246,8 +244,9 @@ describe('patchExpressModule', () => { it('debug error when patching fails', () => { const expressv5 = getExpress5(); - patchExpressModule({ express: expressv5 }); - patchExpressModule({ express: expressv5 }); + const getOptions = () => ({}); + patchExpressModule(expressv5, getOptions); + patchExpressModule(expressv5, getOptions); expect(debugErrors).toStrictEqual([ ['Failed to patch express route method:', new Error('Attempting to wrap method route multiple times')], ['Failed to patch express use method:', new Error('Attempting to wrap method use multiple times')], diff --git a/packages/core/test/lib/integrations/express/patch-layer.test.ts b/packages/core/test/lib/integrations/express/patch-layer.test.ts index 254ffb79edde..8953955ee373 100644 --- a/packages/core/test/lib/integrations/express/patch-layer.test.ts +++ b/packages/core/test/lib/integrations/express/patch-layer.test.ts @@ -150,12 +150,12 @@ describe('patchLayer', () => { describe('no-ops', () => { it('if layer is missing', () => { // mostly for coverage, verifying it doesn't throw or anything - patchLayer({}); + patchLayer(() => ({})); }); it('if layer.handle is missing', () => { // mostly for coverage, verifying it doesn't throw or anything - patchLayer({}, { handle: null } as unknown as ExpressLayer); + patchLayer(() => ({}), { handle: null } as unknown as ExpressLayer); }); it('if layer already patched', () => { @@ -166,7 +166,7 @@ describe('patchLayer', () => { const layer = { handle: wrapped, } as unknown as ExpressLayer; - patchLayer({}, layer); + patchLayer(() => ({}), layer); expect(layer.handle).toBe(wrapped); }); @@ -177,7 +177,7 @@ describe('patchLayer', () => { const layer = { handle: original, } as unknown as ExpressLayer; - patchLayer({}, layer); + patchLayer(() => ({}), layer); expect(layer.handle).toBe(original); }); @@ -188,7 +188,7 @@ describe('patchLayer', () => { const layer = { handle: original, } as unknown as ExpressLayer; - patchLayer({}, layer); + patchLayer(() => ({}), layer); expect(getOriginalFunction(layer.handle)).toBe(original); }); }); @@ -212,7 +212,7 @@ describe('patchLayer', () => { storeLayer(req, '/:boo'); storeLayer(req, '/:car'); - patchLayer(options, layer); + patchLayer(() => options, layer); layer.handle(req, res); expect(layerHandleOriginal).toHaveBeenCalledOnce(); @@ -244,7 +244,7 @@ describe('patchLayer', () => { storeLayer(req, '/:boo'); storeLayer(req, '/:car'); - patchLayer(options, layer, '/layerPath'); + patchLayer(() => options, layer, '/layerPath'); layer.handle(req, res); expect(onRouteResolved).toHaveBeenCalledExactlyOnceWith('/a/:boo/:car/layerPath'); expect(layerHandleOriginal).toHaveBeenCalledOnce(); @@ -290,7 +290,7 @@ describe('patchLayer', () => { // 'router' → router, 'bound dispatch' → request_handler, other → middleware const layerName = type === 'router' ? 'router' : 'bound dispatch'; const layer = { name: layerName, handle: layerHandleOriginal } as unknown as ExpressLayer; - patchLayer(options, layer, '/c'); + patchLayer(() => options, layer, '/c'); // storeLayer('/c') happens inside the patched handle, before being popped // after handle returns, storedLayers should be back to ['/a', '/b'] @@ -327,7 +327,7 @@ describe('patchLayer', () => { storeLayer(req, '/:boo'); storeLayer(req, '/:car'); - patchLayer(options, layer, '/layerPath'); + patchLayer(() => options, layer, '/layerPath'); expect(getOriginalFunction(layer.handle)).toBe(layerHandleOriginal); expect(layer.handle.x).toBe(true); layer.handle.x = false; @@ -382,7 +382,7 @@ describe('patchLayer', () => { storeLayer(req, '/:boo'); storeLayer(req, '/:car'); - patchLayer(options, layer); + patchLayer(() => options, layer); expect(getOriginalFunction(layer.handle)).toBe(layerHandleOriginal); warnings.length = 0; layer.handle(req, res); @@ -441,7 +441,7 @@ describe('patchLayer', () => { storeLayer(req, '/a'); storeLayer(req, '/b'); - patchLayer(options, layer, '/c'); + patchLayer(() => options, layer, '/c'); layer.handle(req, res); expect(onRouteResolved).toHaveBeenCalledExactlyOnceWith('/a/b/c'); const span = mockSpans[0]; @@ -482,7 +482,7 @@ describe('patchLayer', () => { storeLayer(req, '/a'); storeLayer(req, '/b'); - patchLayer(options, layer, '/c'); + patchLayer(() => options, layer, '/c'); layer.handle(req, res); expect(onRouteResolved).toHaveBeenCalledExactlyOnceWith(undefined); const span = mockSpans[0]; @@ -526,7 +526,7 @@ describe('patchLayer', () => { storeLayer(req, '/a'); storeLayer(req, '/b'); - patchLayer(options, layer, '/c'); + patchLayer(() => options, layer, '/c'); expect(getStoredLayers(req)).toStrictEqual(['/a', '/b']); const callback = vi.fn(() => { @@ -576,7 +576,7 @@ describe('patchLayer', () => { storeLayer(req, '/a'); storeLayer(req, '/b'); - patchLayer(options, layer, '/c'); + patchLayer(() => options, layer, '/c'); expect(getStoredLayers(req)).toStrictEqual(['/a', '/b']); const callback = vi.fn(() => { @@ -622,7 +622,7 @@ describe('patchLayer', () => { storeLayer(req, '/a'); storeLayer(req, '/b'); - patchLayer(options, layer, '/c'); + patchLayer(() => options, layer, '/c'); expect(() => { layer.handle(req, res); }).toThrowError('yur head asplode'); diff --git a/packages/node/src/integrations/tracing/express.ts b/packages/node/src/integrations/tracing/express.ts index eb396b81a6ee..c0f7cbc2414f 100644 --- a/packages/node/src/integrations/tracing/express.ts +++ b/packages/node/src/integrations/tracing/express.ts @@ -48,16 +48,15 @@ export class ExpressInstrumentation extends InstrumentationBase { try { - patchExpressModule({ + patchExpressModule(express, () => ({ ...this.getConfig(), - express, onRouteResolved(route) { const rpcMetadata = getRPCMetadata(context.active()); if (route && rpcMetadata?.type === RPCType.HTTP) { rpcMetadata.route = route; } }, - }); + })); } catch (e) { DEBUG_BUILD && debug.error('Failed to patch express module:', e); } @@ -69,8 +68,7 @@ export class ExpressInstrumentation extends InstrumentationBase { +const _expressIntegration = ((options?: ExpressInstrumentationConfig) => { return { name: INTEGRATION_NAME, setupOnce() { @@ -79,4 +77,4 @@ const _expressInstrumentation = ((options?: ExpressInstrumentationConfig) => { }; }) satisfies IntegrationFn; -export const expressIntegration = defineIntegration(_expressInstrumentation); +export const expressIntegration = defineIntegration(_expressIntegration); From 0be7618b1715d5af2aebfa7be40d63cd6583d8b7 Mon Sep 17 00:00:00 2001 From: isaacs Date: Fri, 10 Apr 2026 09:55:49 -0700 Subject: [PATCH 2/2] fix(core): avoid breaking change in express integration --- .../core/src/integrations/express/index.ts | 49 ++++++++++++++++++- .../core/src/integrations/express/types.ts | 6 +++ .../lib/integrations/express/index.test.ts | 31 +++++++++--- 3 files changed, 77 insertions(+), 9 deletions(-) diff --git a/packages/core/src/integrations/express/index.ts b/packages/core/src/integrations/express/index.ts index f1dd8a301c17..bbb1f8fe8a28 100644 --- a/packages/core/src/integrations/express/index.ts +++ b/packages/core/src/integrations/express/index.ts @@ -60,6 +60,24 @@ import { setSDKProcessingMetadata } from './set-sdk-processing-metadata'; const getExpressExport = (express: ExpressModuleExport): ExpressExport => hasDefaultProp(express) ? express.default : (express as ExpressExport); +function isLegacyOptions( + options: ExpressModuleExport | (ExpressIntegrationOptions & { express: ExpressModuleExport }), +): options is ExpressIntegrationOptions & { express: ExpressModuleExport } { + return !!(options as { express: ExpressModuleExport }).express; +} + +// TODO: remove this deprecation handling in v11 +let didLegacyDeprecationWarning = false; +function deprecationWarning() { + if (!didLegacyDeprecationWarning) { + didLegacyDeprecationWarning = true; + DEBUG_BUILD && + debug.warn( + '[Express] `patchExpressModule(options)` is deprecated. Use `patchExpressModule(moduleExports, getOptions)` instead.', + ); + } +} + /** * This is a portable instrumentatiton function that works in any environment * where Express can be loaded, without depending on OpenTelemetry. @@ -72,7 +90,34 @@ const getExpressExport = (express: ExpressModuleExport): ExpressExport => * Sentry.patchExpressModule(express, () => ({})); * ``` */ -export const patchExpressModule = (moduleExports: ExpressModuleExport, getOptions: () => ExpressIntegrationOptions) => { +export function patchExpressModule( + moduleExports: ExpressModuleExport, + getOptions: () => ExpressIntegrationOptions, +): ExpressModuleExport; +/** + * @deprecated Pass the Express module export as the first argument and options getter as the second argument. + */ +export function patchExpressModule( + options: ExpressIntegrationOptions & { express: ExpressModuleExport }, +): ExpressModuleExport; +export function patchExpressModule( + optionsOrExports: ExpressModuleExport | (ExpressIntegrationOptions & { express: ExpressModuleExport }), + maybeGetOptions?: () => ExpressIntegrationOptions, +): ExpressModuleExport { + let getOptions: () => ExpressIntegrationOptions; + let moduleExports: ExpressModuleExport; + if (!maybeGetOptions && isLegacyOptions(optionsOrExports)) { + const { express, ...options } = optionsOrExports; + moduleExports = express; + getOptions = () => options; + deprecationWarning(); + } else if (typeof maybeGetOptions !== 'function') { + throw new TypeError('`patchExpressModule(moduleExports, getOptions)` requires a `getOptions` callback'); + } else { + getOptions = maybeGetOptions; + moduleExports = optionsOrExports as ExpressModuleExport; + } + // pass in the require() or import() result of express const express = getExpressExport(moduleExports); const routerProto: ExpressRouterv4 | ExpressRouterv5 | undefined = isExpressWithRouterPrototype(express) @@ -153,7 +198,7 @@ export const patchExpressModule = (moduleExports: ExpressModuleExport, getOption } return express; -}; +} /** * An Express-compatible error handler, used by setupExpressErrorHandler diff --git a/packages/core/src/integrations/express/types.ts b/packages/core/src/integrations/express/types.ts index 406b2d460e3f..f2ed5eed6013 100644 --- a/packages/core/src/integrations/express/types.ts +++ b/packages/core/src/integrations/express/types.ts @@ -136,6 +136,12 @@ export type ExpressRouter = { export type IgnoreMatcher = string | RegExp | ((name: string) => boolean); export type ExpressIntegrationOptions = { + /** + * @deprecated Pass the express module as the first argument, and an + * options getter as the second argument to patchExpressModule. + */ + express?: ExpressModuleExport; + /** Ignore specific based on their name */ ignoreLayers?: IgnoreMatcher[]; /** Ignore specific layers based on their type */ diff --git a/packages/core/test/lib/integrations/express/index.test.ts b/packages/core/test/lib/integrations/express/index.test.ts index 9e464b1b0adf..7b6ce83120a7 100644 --- a/packages/core/test/lib/integrations/express/index.test.ts +++ b/packages/core/test/lib/integrations/express/index.test.ts @@ -52,8 +52,10 @@ vi.mock('../../../../src/debug-build', () => ({ DEBUG_BUILD: true, })); const debugErrors: [string, Error][] = []; +const debugWarnings: string[] = []; vi.mock('../../../../src/utils/debug-logger', () => ({ debug: { + warn: (msg: string) => debugWarnings.push(msg), error: (msg: string, er: Error) => { debugErrors.push([msg, er]); }, @@ -129,24 +131,34 @@ function getExpress5(): ExpressExportv5 & { spies: ExpressSpies } { } describe('patchExpressModule', () => { - it('throws trying to patch/unpatch the wrong thing', () => { + it('throws trying to patch the wrong thing', () => { expect(() => { patchExpressModule({} as unknown as ExpressModuleExport, () => ({})); }).toThrowError('no valid Express route function to instrument'); }); - it('can patch and restore expressv4 style module', () => { + it('throws trying to patch without a getOptions getter', () => { + const express = getExpress4(); + expect(() => { + //@ts-expect-error The type error prevents this, by design + patchExpressModule(express); + }).toThrowError('`patchExpressModule(moduleExports, getOptions)` requires a `getOptions` callback'); + }); + + it('can patch expressv4 style module', () => { for (const useDefault of [false, true]) { const express = getExpress4(); const moduleExports = useDefault ? { default: express } : express; const r = express.Router as ExpressRouterv4; const a = express.application; - const getOptions = () => ({}); expect((r.use as WrappedFunction).__sentry_original__).toBe(undefined); expect((r.route as WrappedFunction).__sentry_original__).toBe(undefined); expect((a.use as WrappedFunction).__sentry_original__).toBe(undefined); - patchExpressModule(moduleExports, getOptions); + patchExpressModule({ express: moduleExports }); + expect(debugWarnings).toStrictEqual([ + '[Express] `patchExpressModule(options)` is deprecated. Use `patchExpressModule(moduleExports, getOptions)` instead.', + ]); expect(typeof (r.use as WrappedFunction).__sentry_original__).toBe('function'); expect(typeof (r.route as WrappedFunction).__sentry_original__).toBe('function'); @@ -154,18 +166,23 @@ describe('patchExpressModule', () => { } }); - it('can patch and restore expressv5 style module', () => { + it('can patch expressv5 style module', () => { for (const useDefault of [false, true]) { const express = getExpress5(); const r = express.Router as ExpressRouterv5; const a = express.application; const moduleExports = useDefault ? { default: express } : express; - const getOptions = () => ({}); expect((r.prototype.use as WrappedFunction).__sentry_original__).toBe(undefined); expect((r.prototype.route as WrappedFunction).__sentry_original__).toBe(undefined); expect((a.use as WrappedFunction).__sentry_original__).toBe(undefined); - patchExpressModule(moduleExports, getOptions); + // verify that the debug warning doesn't fire a second time + // vitest doesn't guarantee test ordering, so just verify + // in both places that there's only one warning. + patchExpressModule({ express: moduleExports }); + expect(debugWarnings).toStrictEqual([ + '[Express] `patchExpressModule(options)` is deprecated. Use `patchExpressModule(moduleExports, getOptions)` instead.', + ]); expect(typeof (r.prototype.use as WrappedFunction).__sentry_original__).toBe('function'); expect(typeof (r.prototype.route as WrappedFunction).__sentry_original__).toBe('function');