From 0e1f578c60dc39926bc93394e07e77fc6a3f6922 Mon Sep 17 00:00:00 2001 From: Luca Forstner Date: Wed, 25 May 2022 17:26:03 +0200 Subject: [PATCH] feat(nextjs): Make tracing package treeshakable (#5166) --- MIGRATION.md | 2 ++ .../sentry-performance.ts | 17 ++++++++- packages/nextjs/src/index.client.ts | 35 ++++++++++--------- .../tracing/src/browser/browsertracing.ts | 12 ++++--- packages/tracing/src/browser/index.ts | 2 +- packages/tracing/src/index.ts | 12 +++++-- packages/tracing/test/index.bundle.test.ts | 9 ++++- packages/tracing/test/index.test.ts | 9 ++++- 8 files changed, 70 insertions(+), 28 deletions(-) diff --git a/MIGRATION.md b/MIGRATION.md index d6aad039ecaf..58b8e754ded3 100644 --- a/MIGRATION.md +++ b/MIGRATION.md @@ -351,6 +351,8 @@ For our efforts to reduce bundle size of the SDK we had to remove and refactor p - Removed `eventStatusFromHttpCode` to save on bundle size. - Replace `BrowserTracing` `maxTransactionDuration` option with `finalTimeout` option - Removed `ignoreSentryErrors` option from AWS lambda SDK. Errors originating from the SDK will now *always* be caught internally. +- Removed `Integrations.BrowserTracing` export from `@sentry/nextjs`. Please import `BrowserTracing` from `@sentry/nextjs` directly. +- Removed static `id` property from `BrowserTracing` integration. ## Sentry Angular SDK Changes diff --git a/packages/ember/addon/instance-initializers/sentry-performance.ts b/packages/ember/addon/instance-initializers/sentry-performance.ts index 5bb3024f7df6..8f992f8716cc 100644 --- a/packages/ember/addon/instance-initializers/sentry-performance.ts +++ b/packages/ember/addon/instance-initializers/sentry-performance.ts @@ -380,7 +380,22 @@ export async function instrumentForPerformance(appInstance: ApplicationInstance) }), ]; - if (isTesting() && Sentry.getCurrentHub()?.getIntegration(tracing.Integrations.BrowserTracing)) { + class FakeBrowserTracingClass { + static id = tracing.BROWSER_TRACING_INTEGRATION_ID; + public name = FakeBrowserTracingClass.id; + setupOnce() { + // noop - We're just faking this class for a lookup + } + } + + if ( + isTesting() && + Sentry.getCurrentHub()?.getIntegration( + // This is a temporary hack because the BrowserTracing integration cannot have a static `id` field for tree + // shaking reasons. However, `getIntegration` needs that field. + FakeBrowserTracingClass, + ) + ) { // Initializers are called more than once in tests, causing the integrations to not be setup correctly. return; } diff --git a/packages/nextjs/src/index.client.ts b/packages/nextjs/src/index.client.ts index 05b4dc64a3f3..b5993fb734ab 100644 --- a/packages/nextjs/src/index.client.ts +++ b/packages/nextjs/src/index.client.ts @@ -1,4 +1,4 @@ -import { configureScope, init as reactInit, Integrations as BrowserIntegrations } from '@sentry/react'; +import { configureScope, init as reactInit, Integrations } from '@sentry/react'; import { BrowserTracing, defaultRequestInstrumentationOptions } from '@sentry/tracing'; import { EventProcessor } from '@sentry/types'; @@ -10,12 +10,8 @@ import { addIntegration, UserIntegrations } from './utils/userIntegrations'; export * from '@sentry/react'; export { nextRouterInstrumentation } from './performance/client'; -export const Integrations = { ...BrowserIntegrations, BrowserTracing }; +export { Integrations }; -// This is already exported as part of `Integrations` above (and for the moment will remain so for -// backwards compatibility), but that interferes with treeshaking, so we also export it separately -// here. -// // Previously we expected users to import `BrowserTracing` like this: // // import { Integrations } from '@sentry/nextjs'; @@ -28,16 +24,23 @@ export const Integrations = { ...BrowserIntegrations, BrowserTracing }; // const instance = new BrowserTracing(); export { BrowserTracing }; +// Treeshakable guard to remove all code related to tracing +declare const __SENTRY_TRACING__: boolean; + /** Inits the Sentry NextJS SDK on the browser with the React SDK. */ export function init(options: NextjsOptions): void { buildMetadata(options, ['nextjs', 'react']); options.environment = options.environment || process.env.NODE_ENV; - // Only add BrowserTracing if a tracesSampleRate or tracesSampler is set - const integrations = - options.tracesSampleRate === undefined && options.tracesSampler === undefined - ? options.integrations - : createClientIntegrations(options.integrations); + let integrations = options.integrations; + + // Guard below evaluates to true unless __SENTRY_TRACING__ is text-replaced with "false" + if (typeof __SENTRY_TRACING__ === 'undefined' || __SENTRY_TRACING__) { + // Only add BrowserTracing if a tracesSampleRate or tracesSampler is set + if (options.tracesSampleRate !== undefined || options.tracesSampler !== undefined) { + integrations = createClientIntegrations(options.integrations); + } + } reactInit({ ...options, @@ -53,12 +56,12 @@ export function init(options: NextjsOptions): void { }); } -const defaultBrowserTracingIntegration = new BrowserTracing({ - tracingOrigins: [...defaultRequestInstrumentationOptions.tracingOrigins, /^(api\/)/], - routingInstrumentation: nextRouterInstrumentation, -}); - function createClientIntegrations(integrations?: UserIntegrations): UserIntegrations { + const defaultBrowserTracingIntegration = new BrowserTracing({ + tracingOrigins: [...defaultRequestInstrumentationOptions.tracingOrigins, /^(api\/)/], + routingInstrumentation: nextRouterInstrumentation, + }); + if (integrations) { return addIntegration(defaultBrowserTracingIntegration, integrations, { BrowserTracing: { keyPath: 'options.routingInstrumentation', value: nextRouterInstrumentation }, diff --git a/packages/tracing/src/browser/browsertracing.ts b/packages/tracing/src/browser/browsertracing.ts index 1872a08d728f..8995775a8a41 100644 --- a/packages/tracing/src/browser/browsertracing.ts +++ b/packages/tracing/src/browser/browsertracing.ts @@ -15,6 +15,8 @@ import { } from './request'; import { instrumentRoutingWithDefaults } from './router'; +export const BROWSER_TRACING_INTEGRATION_ID = 'BrowserTracing'; + /** Options for Browser Tracing integration */ export interface BrowserTracingOptions extends RequestInstrumentationOptions { /** @@ -111,10 +113,10 @@ const DEFAULT_BROWSER_TRACING_OPTIONS = { * any routing library. This integration uses {@see IdleTransaction} to create transactions. */ export class BrowserTracing implements Integration { - /** - * @inheritDoc - */ - public static id: string = 'BrowserTracing'; + // This class currently doesn't have a static `id` field like the other integration classes, because it prevented + // @sentry/tracing from being treeshaken. Tree shakers do not like static fields, because they behave like side effects. + // TODO: Come up with a better plan, than using static fields on integration classes, and use that plan on all + // integrations. /** Browser Tracing integration options */ public options: BrowserTracingOptions; @@ -122,7 +124,7 @@ export class BrowserTracing implements Integration { /** * @inheritDoc */ - public name: string = BrowserTracing.id; + public name: string = BROWSER_TRACING_INTEGRATION_ID; private _getCurrentHub?: () => Hub; diff --git a/packages/tracing/src/browser/index.ts b/packages/tracing/src/browser/index.ts index 2f7a3fe0d522..dcf4a08270fd 100644 --- a/packages/tracing/src/browser/index.ts +++ b/packages/tracing/src/browser/index.ts @@ -1,4 +1,4 @@ export type { RequestInstrumentationOptions } from './request'; -export { BrowserTracing } from './browsertracing'; +export { BrowserTracing, BROWSER_TRACING_INTEGRATION_ID } from './browsertracing'; export { instrumentOutgoingRequests, defaultRequestInstrumentationOptions } from './request'; diff --git a/packages/tracing/src/index.ts b/packages/tracing/src/index.ts index 0085ad5f6b0a..14d80fb7b4cc 100644 --- a/packages/tracing/src/index.ts +++ b/packages/tracing/src/index.ts @@ -22,7 +22,7 @@ export { Integrations }; // const instance = new BrowserTracing(); // // For an example of of the new usage of BrowserTracing, see @sentry/nextjs index.client.ts -export { BrowserTracing } from './browser'; +export { BrowserTracing, BROWSER_TRACING_INTEGRATION_ID } from './browser'; export { Span, spanStatusfromHttpCode } from './span'; // eslint-disable-next-line deprecation/deprecation @@ -32,8 +32,14 @@ export { instrumentOutgoingRequests, defaultRequestInstrumentationOptions } from export { IdleTransaction } from './idletransaction'; export { startIdleTransaction } from './hubextensions'; -// We are patching the global object with our hub extension methods -addExtensionMethods(); +// Treeshakable guard to remove all code related to tracing +declare const __SENTRY_TRACING__: boolean; + +// Guard for tree +if (typeof __SENTRY_TRACING__ === 'undefined' || __SENTRY_TRACING__) { + // We are patching the global object with our hub extension methods + addExtensionMethods(); +} export { addExtensionMethods }; diff --git a/packages/tracing/test/index.bundle.test.ts b/packages/tracing/test/index.bundle.test.ts index 319daa826ce6..91f643128d21 100644 --- a/packages/tracing/test/index.bundle.test.ts +++ b/packages/tracing/test/index.bundle.test.ts @@ -3,7 +3,14 @@ import { Integrations } from '../src/index.bundle'; describe('Integrations export', () => { it('is exported correctly', () => { Object.keys(Integrations).forEach(key => { - expect(Integrations[key as keyof typeof Integrations].id).toStrictEqual(expect.any(String)); + // Skip BrowserTracing because it doesn't have a static id field. + if (key === 'BrowserTracing') { + return; + } + + expect(Integrations[key as keyof Omit].id).toStrictEqual( + expect.any(String), + ); }); }); }); diff --git a/packages/tracing/test/index.test.ts b/packages/tracing/test/index.test.ts index 8837e2063cc7..c01b8cad0b54 100644 --- a/packages/tracing/test/index.test.ts +++ b/packages/tracing/test/index.test.ts @@ -12,7 +12,14 @@ describe('index', () => { describe('Integrations', () => { it('is exported correctly', () => { Object.keys(Integrations).forEach(key => { - expect(Integrations[key as keyof typeof Integrations].id).toStrictEqual(expect.any(String)); + // Skip BrowserTracing because it doesn't have a static id field. + if (key === 'BrowserTracing') { + return; + } + + expect(Integrations[key as keyof Omit].id).toStrictEqual( + expect.any(String), + ); }); });