Skip to content

feat(integrations): Export pluggable integrations directly #8842

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

feat(integrations): Export pluggable integrations directly #8842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions packages/browser-integration-tests/suites/integrations/exportFromBrowser/init.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import * as Sentry from '@sentry/browser';

window.Sentry = Sentry;

Sentry.init({
dsn: 'https://[email protected]/1337',
integrations: [new Sentry.Integrations.ExtraErrorData()],
});
5 changes: 5 additions & 0 deletions packages/browser-integration-tests/suites/integrations/exportFromBrowser/subject.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
const error = new TypeError('foo');
error.baz = 42;
error.foo = 'bar';

Sentry.captureException(error);
7 changes: 7 additions & 0 deletions packages/browser-integration-tests/suites/integrations/exportFromBrowser/template.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8" />
</head>
<body></body>
</html>
45 changes: 45 additions & 0 deletions packages/browser-integration-tests/suites/integrations/exportFromBrowser/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
import { expect } from '@playwright/test';

import { sentryTest } from '../../../utils/fixtures';
import { envelopeRequestParser, waitForErrorRequestOnUrl } from '../../../utils/helpers';

sentryTest('allows to use pluggable integrations from @sentry/browser', async ({ getLocalTestUrl, page }) => {
const bundle = process.env.PW_BUNDLE;

// Only run this for import-based tests, not CDN bundles/loader
if (bundle && bundle.startsWith('bundle_')) {
sentryTest.skip();
}

const url = await getLocalTestUrl({ testDir: __dirname });

const req = await waitForErrorRequestOnUrl(page, url);

const eventData = envelopeRequestParser(req);

expect(eventData).toEqual(
expect.objectContaining({
contexts: expect.objectContaining({
TypeError: {
baz: 42,
foo: 'bar',
},
}),
exception: {
values: [
{
type: 'TypeError',
value: 'foo',
mechanism: {
type: 'generic',
handled: true,
},
stacktrace: {
frames: expect.any(Array),
},
},
],
},
}),
);
});
10 changes: 8 additions & 2 deletions packages/browser-integration-tests/utils/generatePlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,14 @@ class SentryScenarioGenerationPlugin {
// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
parser.hooks.import.tap(
this._name,
(statement: { specifiers: [{ imported: { name: string } }] }, source: string) => {
if (source === '@sentry/integrations') {
(statement: { specifiers: [{ imported?: { name: string }; name?: string }] }, source: string) => {
// We only want to handle the integrations import if it doesn't come from the @sentry/browser re-export
// In that case, we just want to leave it alone
if (
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without this change, this fails for all tests as the build output of webpack includes the import from @sentry/integrations by default and tries to "fix" it in there, but it also does not contain an imported field so it just fails.

source === '@sentry/integrations' &&
statement.specifiers[0].name !== 'PluggableIntegrations' &&
statement.specifiers[0].imported
) {
this.requiredIntegrations.push(statement.specifiers[0].imported.name.toLowerCase());
} else if (source === '@sentry/wasm') {
this.requiresWASMIntegration = true;
Expand Down
1 change: 1 addition & 0 deletions packages/browser/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"dependencies": {
"@sentry-internal/tracing": "7.64.0",
"@sentry/core": "7.64.0",
"@sentry/integrations": "7.64.0",
"@sentry/replay": "7.64.0",
"@sentry/types": "7.64.0",
"@sentry/utils": "7.64.0",
Expand Down
2 changes: 2 additions & 0 deletions packages/browser/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
export * from './exports';

import { Integrations as CoreIntegrations } from '@sentry/core';
import * as PluggableIntegrations from '@sentry/integrations';

import { WINDOW } from './helpers';
import * as BrowserIntegrations from './integrations';
Expand All @@ -16,6 +17,7 @@ const INTEGRATIONS = {
...windowIntegrations,
...CoreIntegrations,
...BrowserIntegrations,
...PluggableIntegrations,
Copy link
Contributor

@lforst lforst Aug 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

h: This will not tree-shake as soon as people import Integrations (and not even access one of the pluggable ones) because the spread operator (or in fact assigning an object) is a runtime operation. It's for the same reason we have BrowserTracing as a direct export and not under Integrations.

We need to do the same here - exporting the pluggable integrations top level.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

right, that makes sense of course... I wonder actually if we should then completely get rid of the Integrations key and move all of these to the top level instead? 🤔 And then maybe suffix them with Integration or something like this...:

export { Dedupe as DedupeIntegration } from '@sentry/integrations';
// etc

IMHO it would be a bit weird to have the default integrations be available under Integrations and the pluggable ones directly at the root 😅

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the next major we should remove the Integrations object for sure. I agree having them in separate places is weird. I think we could export everything top level.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#8844

};

export { INTEGRATIONS as Integrations };
Expand Down
1 change: 0 additions & 1 deletion packages/integrations/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@
"tslib": "^2.4.1 || ^1.9.3"
},
"devDependencies": {
"@sentry/browser": "7.64.0",
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would have been a circular dependency, so got rid of this (similar to what we had to do for replay)

"chai": "^4.1.2"
},
"scripts": {
Expand Down
6 changes: 5 additions & 1 deletion packages/integrations/test/offline.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
/* eslint-disable deprecation/deprecation */
import { WINDOW } from '@sentry/browser';
import type { Event, EventProcessor, Hub, Integration, IntegrationClass } from '@sentry/types';
import { GLOBAL_OBJ } from '@sentry/utils';

import type { Item } from '../src/offline';
import { Offline } from '../src/offline';

// exporting a separate copy of `WINDOW` rather than exporting the one from `@sentry/browser`
// to avoid a circular dependency (as `@sentry/browser` imports `@sentry/integrations`)
const WINDOW = GLOBAL_OBJ as typeof GLOBAL_OBJ & Window;

// mock localforage methods
jest.mock('localforage', () => ({
createInstance(_options: { name: string }): any {
Expand Down
12 changes: 12 additions & 0 deletions packages/node-integration-tests/suites/integrations/exportFromNode/scenario.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
import * as Sentry from '@sentry/node';

Sentry.init({
dsn: 'https://[email protected]/1337',
integrations: [new Sentry.Integrations.ExtraErrorData({})],
});

const error = new TypeError('foo') as Error & { baz: number; foo: string };
error.baz = 42;
error.foo = 'bar';

Sentry.captureException(error);
30 changes: 30 additions & 0 deletions packages/node-integration-tests/suites/integrations/exportFromNode/test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
import { assertSentryEvent, TestEnv } from '../../../utils';

test('allows to use pluggable integrations from @sentry/node export', async () => {
const env = await TestEnv.init(__dirname);
const event = await env.getEnvelopeRequest();

assertSentryEvent(event[2], {
contexts: expect.objectContaining({
TypeError: {
baz: 42,
foo: 'bar',
},
}),
exception: {
values: [
{
type: 'TypeError',
value: 'foo',
mechanism: {
type: 'generic',
handled: true,
},
stacktrace: {
frames: expect.any(Array),
},
},
],
},
});
});
1 change: 1 addition & 0 deletions packages/node/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
"dependencies": {
"@sentry-internal/tracing": "7.64.0",
"@sentry/core": "7.64.0",
"@sentry/integrations": "7.64.0",
"@sentry/types": "7.64.0",
"@sentry/utils": "7.64.0",
"cookie": "^0.4.1",
Expand Down
2 changes: 2 additions & 0 deletions packages/node/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ export { deepReadDirSync } from './utils';
export { getModuleFromFilename } from './module';

import { Integrations as CoreIntegrations } from '@sentry/core';
import * as PluggableIntegrations from '@sentry/integrations';

import * as Handlers from './handlers';
import * as NodeIntegrations from './integrations';
Expand All @@ -79,6 +80,7 @@ const INTEGRATIONS = {
...CoreIntegrations,
...NodeIntegrations,
...TracingIntegrations,
...PluggableIntegrations,
};

export { INTEGRATIONS as Integrations, Handlers };