fix(core): avoid breaking change in express integration

Author: isaacs

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

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,43 +131,58 @@ 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');
       expect(typeof (a.use as WrappedFunction).__sentry_original__).toBe('function');
     }
   });
 
-  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');