Skip to content

feat: Stop passing defaultIntegrations as client option #15307

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.

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

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ sentryTest(
if (hasDebugLogs()) {
expect(errorLogs.length).toEqual(1);
expect(errorLogs[0]).toEqual(
'[Sentry] You cannot run Sentry this way in a browser extension, check: https://docs.sentry.io/platforms/javascript/best-practices/browser-extensions/',
'[Sentry] You cannot use Sentry.init() in a browser extension, see: https://docs.sentry.io/platforms/javascript/best-practices/browser-extensions/',
);
} else {
expect(errorLogs.length).toEqual(0);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ sentryTest('should not initialize when inside a Chrome browser extension', async
if (hasDebugLogs()) {
expect(errorLogs.length).toEqual(1);
expect(errorLogs[0]).toEqual(
'[Sentry] You cannot run Sentry this way in a browser extension, check: https://docs.sentry.io/platforms/javascript/best-practices/browser-extensions/',
'[Sentry] You cannot use Sentry.init() in a browser extension, see: https://docs.sentry.io/platforms/javascript/best-practices/browser-extensions/',
);
} else {
expect(errorLogs.length).toEqual(0);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const NODE_EXPORTS_IGNORE = [
'setNodeAsyncContextStrategy',
'getDefaultIntegrationsWithoutPerformance',
'initWithoutDefaultIntegrations',
'initWithDefaultIntegrations',
'SentryContextManager',
'validateOpenTelemetrySetup',
'preloadOpenTelemetry',
Expand Down
6 changes: 3 additions & 3 deletions packages/angular/src/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import {
browserSessionIntegration,
globalHandlersIntegration,
httpContextIntegration,
init as browserInit,
initWithDefaultIntegrations,
linkedErrorsIntegration,
setContext,
} from '@sentry/browser';
Expand Down Expand Up @@ -50,14 +50,14 @@ export function getDefaultIntegrations(_options: BrowserOptions = {}): Integrati
*/
export function init(options: BrowserOptions): Client | undefined {
const opts = {
defaultIntegrations: getDefaultIntegrations(),
...options,
};

applySdkMetadata(opts, 'angular');

checkAndSetAngularVersion();
return browserInit(opts);

return initWithDefaultIntegrations(opts, getDefaultIntegrations);
Comment on lines 58 to +60
Copy link
Member

Choose a reason for hiding this comment

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

now that I see this: is it even correct to call checkAndSetAngularVersion before initWithDefaultIntegrations (or formerly browserInit)? Given that this calls setContext before the actual client exists...

I guess it somehow works but probably it's better to first call init, then checkSetNgVersion and then return the client

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 should be fine because the scope remains valid even before the client is created (one of the particular design goals we considered with the current scope stuff 😅 )

}

function checkAndSetAngularVersion(): void {
Expand Down
5 changes: 2 additions & 3 deletions packages/astro/src/client/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import type { BrowserOptions } from '@sentry/browser';
import {
browserTracingIntegration,
getDefaultIntegrations as getBrowserDefaultIntegrations,
init as initBrowserSdk,
initWithDefaultIntegrations,
} from '@sentry/browser';
import type { Client, Integration } from '@sentry/core';
import { applySdkMetadata } from '@sentry/core';
Expand All @@ -17,13 +17,12 @@ declare const __SENTRY_TRACING__: boolean;
*/
export function init(options: BrowserOptions): Client | undefined {
const opts = {
defaultIntegrations: getDefaultIntegrations(options),
...options,
};

applySdkMetadata(opts, 'astro', ['astro', 'browser']);

return initBrowserSdk(opts);
return initWithDefaultIntegrations(opts, getDefaultIntegrations);
}

function getDefaultIntegrations(options: BrowserOptions): Integration[] {
Expand Down
4 changes: 4 additions & 0 deletions packages/astro/src/index.types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,10 @@ export * from '@sentry/node';

/** Initializes Sentry Astro SDK */
export declare function init(options: Options | clientSdk.BrowserOptions | NodeOptions): Client | undefined;
export declare function initWithDefaultIntegrations(
options: Options | clientSdk.BrowserOptions | NodeOptions,
getDefaultIntegrations: (options: Options) => Integration[],
): Client | undefined;

export declare const linkedErrorsIntegration: typeof clientSdk.linkedErrorsIntegration;
export declare const contextLinesIntegration: typeof clientSdk.contextLinesIntegration;
Expand Down
23 changes: 8 additions & 15 deletions packages/astro/test/client/sdk.test.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
import type { BrowserClient } from '@sentry/browser';
import {
browserTracingIntegration,
getActiveSpan,
getClient,
getCurrentScope,
getGlobalScope,
getIsolationScope,
Expand All @@ -12,7 +10,7 @@ import * as SentryBrowser from '@sentry/browser';
import { afterEach, describe, expect, it, vi } from 'vitest';
import { init } from '../../src/client/sdk';

const browserInit = vi.spyOn(SentryBrowser, 'init');
const browserInit = vi.spyOn(SentryBrowser, 'initWithDefaultIntegrations');

describe('Sentry client SDK', () => {
describe('init', () => {
Expand Down Expand Up @@ -44,6 +42,7 @@ describe('Sentry client SDK', () => {
},
},
}),
expect.any(Function),
);
});

Expand All @@ -53,45 +52,39 @@ describe('Sentry client SDK', () => {
['tracesSampler', { tracesSampler: () => 1.0 }],
['no tracing option set', {}],
])('adds browserTracingIntegration if tracing is enabled via %s', (_, tracingOptions) => {
init({
const client = init({
dsn: 'https://[email protected]/1337',
...tracingOptions,
});

const integrationsToInit = browserInit.mock.calls[0]![0]?.defaultIntegrations;
const browserTracing = getClient<BrowserClient>()?.getIntegrationByName('BrowserTracing');

expect(integrationsToInit).toContainEqual(expect.objectContaining({ name: 'BrowserTracing' }));
const browserTracing = client?.getIntegrationByName('BrowserTracing');
expect(browserTracing).toBeDefined();
});

it("doesn't add browserTracingIntegration if `__SENTRY_TRACING__` is set to false", () => {
(globalThis as any).__SENTRY_TRACING__ = false;

init({
const client = init({
dsn: 'https://[email protected]/1337',
tracesSampleRate: 1,
});

const integrationsToInit = browserInit.mock.calls[0]![0]?.defaultIntegrations || [];
const browserTracing = getClient<BrowserClient>()?.getIntegrationByName('BrowserTracing');

expect(integrationsToInit).not.toContainEqual(expect.objectContaining({ name: 'BrowserTracing' }));
const browserTracing = client?.getIntegrationByName('BrowserTracing');
expect(browserTracing).toBeUndefined();

delete (globalThis as any).__SENTRY_TRACING__;
});

it('Overrides the automatically default browserTracingIntegration instance with a a user-provided browserTracingIntegration instance', () => {
init({
const client = init({
dsn: 'https://[email protected]/1337',
integrations: [
browserTracingIntegration({ finalTimeout: 10, instrumentNavigation: false, instrumentPageLoad: false }),
],
tracesSampleRate: 1,
});

const browserTracing = getClient<BrowserClient>()?.getIntegrationByName('BrowserTracing');
const browserTracing = client?.getIntegrationByName('BrowserTracing');
expect(browserTracing).toBeDefined();

// no active span means the settings were respected
Expand Down
5 changes: 2 additions & 3 deletions packages/aws-serverless/src/sdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import {
flush,
getCurrentScope,
getDefaultIntegrationsWithoutPerformance,
initWithoutDefaultIntegrations,
initWithDefaultIntegrations,
startSpanManual,
withScope,
} from '@sentry/node';
Expand Down Expand Up @@ -77,13 +77,12 @@ export function getDefaultIntegrations(_options: Options): Integration[] {
*/
export function init(options: NodeOptions = {}): NodeClient | undefined {
const opts = {
defaultIntegrations: getDefaultIntegrations(options),
...options,
};

applySdkMetadata(opts, 'aws-serverless');

return initWithoutDefaultIntegrations(opts);
return initWithDefaultIntegrations(opts, getDefaultIntegrations);
}

/** */
Expand Down
2 changes: 1 addition & 1 deletion packages/aws-serverless/test/sdk.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ vi.mock('@sentry/node', async () => {
const original = (await vi.importActual('@sentry/node')) as typeof import('@sentry/node');
return {
...original,
initWithoutDefaultIntegrations: (options: unknown) => {
initWithDefaultIntegrations: (options: unknown) => {
mockInit(options);
},
startInactiveSpan: (...args: unknown[]) => {
Expand Down
2 changes: 2 additions & 0 deletions packages/browser/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ export * from './exports';

export { logger };

export { initWithDefaultIntegrations } from './sdk';

export { reportingObserverIntegration } from './integrations/reportingobserver';
export { httpClientIntegration } from './integrations/httpclient';
export { contextLinesIntegration } from './integrations/contextlines';
Expand Down
Loading
Loading