fix(core, node): support loading Express options lazily#20211
Open
fix(core, node): support loading Express options lazily#20211
Conversation
Contributor
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. New Features ✨Core
Deps
Other
Bug Fixes 🐛Deno
Other
Internal Changes 🔧Deps
Other
🤖 This preview updates automatically when you update the PR. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Autofix Details
Bugbot Autofix prepared a fix for the issue found in the latest run.
- ✅ Fixed: Breaking public API change without deprecation notice
- I restored backward compatibility by reintroducing the deprecated legacy
patchExpressModule(options)overload andExpressIntegrationOptions.expressfield with a deprecation warning, and added a regression test for the old call pattern.
- I restored backward compatibility by reintroducing the deprecated legacy
Or push these changes by commenting:
@cursor push eebe164287
Preview (eebe164287)
diff --git a/packages/core/src/integrations/express/index.ts b/packages/core/src/integrations/express/index.ts
--- a/packages/core/src/integrations/express/index.ts
+++ b/packages/core/src/integrations/express/index.ts
@@ -60,6 +60,12 @@
const getExpressExport = (express: ExpressModuleExport): ExpressExport =>
hasDefaultProp(express) ? express.default : (express as ExpressExport);
+type LegacyPatchExpressModuleOptions = ExpressIntegrationOptions & {
+ express: ExpressModuleExport;
+};
+
+let _didWarnLegacyPatchExpressModule = false;
+
/**
* This is a portable instrumentatiton function that works in any environment
* where Express can be loaded, without depending on OpenTelemetry.
@@ -72,7 +78,46 @@
* Sentry.patchExpressModule(express, () => ({}));
* ```
*/
-export const patchExpressModule = (moduleExports: ExpressModuleExport, getOptions: () => ExpressIntegrationOptions) => {
+export function patchExpressModule(
+ moduleExports: ExpressModuleExport,
+ getOptions: () => ExpressIntegrationOptions,
+): ExpressExport;
+/**
+ * @deprecated Pass the Express module export as the first argument and options getter as the second argument.
+ */
+export function patchExpressModule(options: LegacyPatchExpressModuleOptions): ExpressExport;
+export function patchExpressModule(
+ moduleExportsOrOptions: ExpressModuleExport | LegacyPatchExpressModuleOptions,
+ getOptions?: () => ExpressIntegrationOptions,
+): ExpressExport {
+ const isLegacyOptionsObject =
+ !getOptions &&
+ typeof moduleExportsOrOptions === 'object' &&
+ moduleExportsOrOptions !== null &&
+ 'express' in moduleExportsOrOptions;
+
+ let moduleExports: ExpressModuleExport;
+ let getExpressOptions: () => ExpressIntegrationOptions;
+
+ if (isLegacyOptionsObject) {
+ moduleExports = moduleExportsOrOptions.express;
+ getExpressOptions = () => moduleExportsOrOptions;
+ } else {
+ moduleExports = moduleExportsOrOptions;
+ if (!getOptions) {
+ throw new TypeError('`patchExpressModule(moduleExports, getOptions)` requires a `getOptions` callback');
+ }
+ getExpressOptions = getOptions;
+ }
+
+ if (isLegacyOptionsObject && !_didWarnLegacyPatchExpressModule) {
+ _didWarnLegacyPatchExpressModule = true;
+ DEBUG_BUILD &&
+ debug.warn(
+ '[Express] `patchExpressModule(options)` is deprecated. Use `patchExpressModule(moduleExports, getOptions)` instead.',
+ );
+ }
+
// pass in the require() or import() result of express
const express = getExpressExport(moduleExports);
const routerProto: ExpressRouterv4 | ExpressRouterv5 | undefined = isExpressWithRouterPrototype(express)
@@ -94,7 +139,7 @@
function routeTrace(this: ExpressRouter, ...args: Parameters<typeof originalRouteMethod>[]) {
const route = originalRouteMethod.apply(this, args);
const layer = this.stack[this.stack.length - 1] as ExpressLayer;
- patchLayer(getOptions, layer, getLayerPath(args));
+ patchLayer(getExpressOptions, layer, getLayerPath(args));
return route;
},
);
@@ -114,7 +159,7 @@
if (!layer) {
return route;
}
- patchLayer(getOptions, layer, getLayerPath(args));
+ patchLayer(getExpressOptions, layer, getLayerPath(args));
return route;
},
);
@@ -142,7 +187,7 @@
if (router) {
const layer = router.stack[router.stack.length - 1];
if (layer) {
- patchLayer(getOptions, layer, getLayerPath(args));
+ patchLayer(getExpressOptions, layer, getLayerPath(args));
}
}
return route;
@@ -153,7 +198,7 @@
}
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
--- a/packages/core/src/integrations/express/types.ts
+++ b/packages/core/src/integrations/express/types.ts
@@ -136,6 +136,10 @@
export type IgnoreMatcher = string | RegExp | ((name: string) => boolean);
export type ExpressIntegrationOptions = {
+ /**
+ * @deprecated Pass the Express module export as the first argument to `patchExpressModule` instead.
+ */
+ 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
--- a/packages/core/test/lib/integrations/express/index.test.ts
+++ b/packages/core/test/lib/integrations/express/index.test.ts
@@ -52,15 +52,22 @@
DEBUG_BUILD: true,
}));
const debugErrors: [string, Error][] = [];
+const debugWarnings: string[] = [];
vi.mock('../../../../src/utils/debug-logger', () => ({
debug: {
error: (msg: string, er: Error) => {
debugErrors.push([msg, er]);
},
+ warn: (msg: string) => {
+ debugWarnings.push(msg);
+ },
},
}));
-beforeEach(() => (patchLayerCalls.length = 0));
+beforeEach(() => {
+ patchLayerCalls.length = 0;
+ debugWarnings.length = 0;
+});
const patchLayerCalls: [getOptions: () => ExpressIntegrationOptions, layer: ExpressLayer, layerPath?: string][] = [];
vi.mock('../../../../src/integrations/express/patch-layer', () => ({
@@ -129,6 +136,18 @@
}
describe('patchExpressModule', () => {
+ it('supports deprecated options signature', () => {
+ const expressv4 = getExpress4();
+ const options = { express: expressv4, ignoreLayers: [/foo/] };
+ patchExpressModule(options);
+ expressv4.Router.use('a');
+
+ expect(patchLayerCalls[0]?.[0]()).toStrictEqual(options);
+ expect(debugWarnings).toStrictEqual([
+ '[Express] `patchExpressModule(options)` is deprecated. Use `patchExpressModule(moduleExports, getOptions)` instead.',
+ ]);
+ });
+
it('throws trying to patch/unpatch the wrong thing', () => {
expect(() => {
patchExpressModule({} as unknown as ExpressModuleExport, () => ({}));This Bugbot Autofix run was free. To enable autofix for future PRs, go to the Cursor dashboard.
Contributor
size-limit report 📦
|
Contributor
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
Member
Author
|
Cursor makes a good point, it is technically breaking, even though it's an internal and undocumented method. I can add a bit to handle that. |
isaacs
force-pushed
the
isaacs/express-late-init
branch
from
7c8de61 to
0be7618
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 0be7618. Configure here.
| } else { | ||
| getOptions = maybeGetOptions; | ||
| moduleExports = optionsOrExports as ExpressModuleExport; | ||
| } |
There was a problem hiding this comment.
Public API function signature changed for exported function
Low Severity
Flagging per the review rules file: patchExpressModule is publicly exported from @sentry/core and its function signature changed from a single-argument form to a two-argument overloaded form. Additionally, the publicly exported ExpressIntegrationOptions type changed the express field from required to optional. While backward compatibility is maintained via a deprecated overload and runtime deprecation warning, consumers who stored options in a variable typed as ExpressIntegrationOptions and passed it to patchExpressModule will encounter a TypeScript error, since the deprecated overload requires ExpressIntegrationOptions & { express: ExpressModuleExport } which the now-optional express field no longer satisfies.
Additional Locations (1)
Triggered by project rule: PR Review Guidelines for Cursor Bot
Reviewed by Cursor Bugbot for commit 0be7618. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Before submitting a pull request, please take a look at our
Contributing guidelines and verify:
yarn lint) & (yarn test).Closes #JS-2117