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

ref(tracing): Make tracing integrations tree shakeable #4204

Merged
merged 18 commits into from
Dec 3, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
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
3 changes: 1 addition & 2 deletions packages/nextjs/src/index.client.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { configureScope, init as reactInit, Integrations as BrowserIntegrations } from '@sentry/react';
import { defaultRequestInstrumentationOptions, Integrations as TracingIntegrations } from '@sentry/tracing';
import { BrowserTracing, defaultRequestInstrumentationOptions } from '@sentry/tracing';

import { nextRouterInstrumentation } from './performance/client';
import { MetadataBuilder } from './utils/metadataBuilder';
Expand All @@ -9,7 +9,6 @@ import { addIntegration, UserIntegrations } from './utils/userIntegrations';
export * from '@sentry/react';
export { nextRouterInstrumentation } from './performance/client';

const { BrowserTracing } = TracingIntegrations;
export const Integrations = { ...BrowserIntegrations, BrowserTracing };

/** Inits the Sentry NextJS SDK on the browser with the React SDK. */
Expand Down
24 changes: 20 additions & 4 deletions packages/tracing/src/index.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,26 @@
import { BrowserTracing } from './browser';
import { addExtensionMethods } from './hubextensions';
import * as TracingIntegrations from './integrations';

const Integrations = { ...TracingIntegrations, BrowserTracing };
import * as Integrations from './integrations';

export { Integrations };

AbhiPrasad marked this conversation as resolved.
Show resolved Hide resolved
// 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 tracing integrations like
//
// import { Integrations } from '@sentry/tracing';
// const instance = new Integrations.BrowserTracing();
//
// This makes the integrations unable to be treeshaken though. To address this, we now have
// this individual export. We now expect users to consume BrowserTracing like so:
//
// import { BrowserTracing } from '@sentry/tracing';
// const instance = new BrowserTracing();
//
// For an example of of the new usage of BrowserTracing, see @sentry/nextjs index.client.ts
export { BrowserTracing } from './browser';
AbhiPrasad marked this conversation as resolved.
Show resolved Hide resolved

export { Span } from './span';
export { Transaction } from './transaction';
export {
Expand Down
4 changes: 4 additions & 0 deletions packages/tracing/src/integrations/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,7 @@ export { Express } from './node/express';
export { Postgres } from './node/postgres';
export { Mysql } from './node/mysql';
export { Mongo } from './node/mongo';

// TODO(v7): Remove this export
// Please see `src/index.ts` for more details.
export { BrowserTracing } from '../browser';
8 changes: 3 additions & 5 deletions packages/tracing/test/index.bundle.test.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
import { Integrations } from '../src/index.bundle';
import { testOnlyIfNodeVersionAtLeast } from './testutils';

describe('Integrations export', () => {
// TODO `Object.values` doesn't work on Node < 8
testOnlyIfNodeVersionAtLeast(8)('is exported correctly', () => {
Object.values(Integrations).forEach(integration => {
expect(integration.id).toStrictEqual(expect.any(String));
it('is exported correctly', () => {
Object.keys(Integrations).forEach(key => {
expect(Integrations[key as keyof typeof Integrations].id).toStrictEqual(expect.any(String));
Comment on lines +4 to +6
Copy link
Member

Choose a reason for hiding this comment

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

In both its old and new versions, I find this test confusing. How does the fact that every value in the Integrations object is itself an object with a string-valued id property prove anything about how things are getting exported? (This applies to the version in index.test.ts also.)

(Also, what's the advantage of the refactor here?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I just made it so that we don't need to use testOnlyIfNodeVersionAtLeast anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah good question on the value of this test. I just copied the existing one from index.bundle.test.ts into index.test.ts, but open to alternatives.

Copy link
Member

Choose a reason for hiding this comment

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

I just made it so that we don't need to use testOnlyIfNodeVersionAtLeast anymore.

I figured that might be the case. My thought about that, though, is that if you leave aside the node restriction, IMHO the old test is simpler, because you don't need the keyof typeof stuff, and since we're about to drop node 6 compatibility anyway, we could just remove the wrapper when we do and still get to have a simpler test. What do you think?

That said, yeah, first we have to figure out if we even want the test in the first place! My issue with it is less "is there value in testing the exportation?" (though I'm iffy on that) and more "okay, but how does any of this prove that it's exported correctly?". Do you understand the logic there?

Copy link
Member Author

Choose a reason for hiding this comment

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

whether to deal with it here or come back to it

I will come back to this and address this in another PR. You make great points that we have to figure out.

});
});
});
23 changes: 23 additions & 0 deletions packages/tracing/test/index.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
import { getCurrentHub } from '@sentry/hub';

import { BrowserTracing, Integrations } from '../src';

describe('index', () => {
it('patches the global hub to add an implementation for `Hub.startTransaction` as a side effect', () => {
const hub = getCurrentHub();
const transaction = hub.startTransaction({ name: 'test', endTimestamp: 123 });
expect(transaction).toBeDefined();
});

describe('Integrations', () => {
it('is exported correctly', () => {
Object.keys(Integrations).forEach(key => {
expect(Integrations[key as keyof typeof Integrations].id).toStrictEqual(expect.any(String));
});
});

it('contains BrowserTracing', () => {
expect(Integrations.BrowserTracing).toEqual(BrowserTracing);
});
});
});