Skip to content
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

feat(nextjs): Make tracing package treeshakable #5166

Merged
merged 6 commits into from
May 25, 2022
Merged
Show file tree
Hide file tree
Changes from 3 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
2 changes: 2 additions & 0 deletions MIGRATION.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
17 changes: 16 additions & 1 deletion packages/ember/addon/instance-initializers/sentry-performance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
35 changes: 19 additions & 16 deletions packages/nextjs/src/index.client.ts
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -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';
Expand All @@ -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,
Expand All @@ -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 },
Expand Down
12 changes: 7 additions & 5 deletions packages/tracing/src/browser/browsertracing.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
/**
Expand Down Expand Up @@ -111,18 +113,18 @@ 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;

/**
* @inheritDoc
*/
public name: string = BrowserTracing.id;
public name: string = BROWSER_TRACING_INTEGRATION_ID;

private _getCurrentHub?: () => Hub;

Expand Down
2 changes: 1 addition & 1 deletion packages/tracing/src/browser/index.ts
Original file line number Diff line number Diff line change
@@ -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';
12 changes: 9 additions & 3 deletions packages/tracing/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 };

Expand Down
5 changes: 5 additions & 0 deletions packages/tracing/test/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,11 @@ describe('index', () => {
describe('Integrations', () => {
it('is exported correctly', () => {
Object.keys(Integrations).forEach(key => {
// Skip BrowserTracing because it doesn't have a static id field.
if (key !== 'BrowserTracing') {
Copy link
Member

Choose a reason for hiding this comment

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

This should be the other way round, right?

Suggested change
if (key !== 'BrowserTracing') {
if (key === 'BrowserTracing') {

Copy link
Member Author

Choose a reason for hiding this comment

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

yup thanks 😄

return;
}

expect(Integrations[key as keyof typeof Integrations].id).toStrictEqual(expect.any(String));
});
});
Expand Down