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.

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

Jump to bottom

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

Merged
merged 18 commits into from
Dec 3, 2021
Merged

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

Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
d8101c0
ref(tracing): Make integrations tree shakeable
AbhiPrasad Dec 1, 2021
8adf3ec
test: Add browser based scenarios (#4100)
AbhiPrasad Dec 1, 2021
71116cc
fix(ember): Restore local and PR tests (#4205)
lobsterkatie Dec 1, 2021
39b5a75
add tests for new integration exports
AbhiPrasad Dec 1, 2021
2cafb72
Merge branch 'master' into abhi-browser-tracing-tree-shake
AbhiPrasad Dec 1, 2021
246ea11
woops
AbhiPrasad Dec 1, 2021
32840bc
Merge branch 'master' into abhi-browser-tracing-tree-shake
AbhiPrasad Dec 1, 2021
fc33c16
Merge branch 'master' into abhi-browser-tracing-tree-shake
AbhiPrasad Dec 1, 2021
fd6c608
Update packages/tracing/test/index.test.ts
AbhiPrasad Dec 1, 2021
0a5363b
Merge branch 'master' into abhi-browser-tracing-tree-shake
AbhiPrasad Dec 1, 2021
8a2699f
Merge branch 'master' into abhi-browser-tracing-tree-shake
AbhiPrasad Dec 1, 2021
34d8376
fix(react): Mark react package as having no side effects (#4213)
AbhiPrasad Dec 2, 2021
751e054
ref(integrations): Make ReportTypes a const enum (#4209)
AbhiPrasad Dec 2, 2021
e50a43f
ref(utils): Make States a const enum (#4210)
AbhiPrasad Dec 2, 2021
65b40d9
add clarifying comments around integrations export:
AbhiPrasad Dec 2, 2021
9339101
move test around
AbhiPrasad Dec 2, 2021
fff4241
Merge branch 'master' into abhi-browser-tracing-tree-shake
AbhiPrasad Dec 2, 2021
04dcc80
Merge branch 'master' into abhi-browser-tracing-tree-shake
AbhiPrasad Dec 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions packages/nextjs/src/index.client.ts
View file
Open in desktop
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
8 changes: 4 additions & 4 deletions packages/tracing/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { BrowserTracing } from './browser';
import { addExtensionMethods } from './hubextensions';
import * as TracingIntegrations from './integrations';

const Integrations = { ...TracingIntegrations, BrowserTracing };
import * as Integrations from './integrationsbundle';
AbhiPrasad marked this conversation as resolved.
Show resolved Hide resolved

export { Integrations };

AbhiPrasad marked this conversation as resolved.
Show resolved Hide resolved
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
2 changes: 2 additions & 0 deletions packages/tracing/src/integrationsbundle.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
export * from './integrations';
export { BrowserTracing } from './browser';
8 changes: 3 additions & 5 deletions packages/tracing/test/index.bundle.test.ts
View file
Open in desktop
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
View file
Open in desktop
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', () => {
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);
});
});

it('patches the global hub as a side effect', () => {
AbhiPrasad marked this conversation as resolved.
Show resolved Hide resolved
const hub = getCurrentHub();
const transaction = hub.startTransaction({ name: 'test', endTimestamp: 123 });
expect(transaction).toBeDefined();
});
});