Skip to content

ref(gatsby): Move SDK initialization logic to SDK #4040

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

Merged
merged 7 commits into from
Oct 11, 2021
Merged
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
26 changes: 1 addition & 25 deletions packages/gatsby/gatsby-browser.js
Original file line number Diff line number Diff line change
@@ -1,40 +1,16 @@
const Sentry = require('@sentry/react');
const Tracing = require('@sentry/tracing');
const Sentry = require('@sentry/gatsby');

exports.onClientEntry = function(_, pluginParams) {
if (pluginParams === undefined) {
return;
}

pluginParams._metadata = pluginParams._metadata || {};
pluginParams._metadata.sdk = {
name: 'sentry.javascript.gatsby',
packages: [
{
name: 'npm:@sentry/gatsby',
version: Sentry.SDK_VERSION,
},
],
version: Sentry.SDK_VERSION,
};

const integrations = [...(pluginParams.integrations || [])];

if (Tracing.hasTracingEnabled(pluginParams) && !integrations.some(ele => ele.name === 'BrowserTracing')) {
integrations.push(new Tracing.Integrations.BrowserTracing(pluginParams.browserTracingOptions));
}

Tracing.addExtensionMethods();

Sentry.init({
autoSessionTracking: true,
environment: process.env.NODE_ENV || 'development',
// eslint-disable-next-line no-undef
release: __SENTRY_RELEASE__,
// eslint-disable-next-line no-undef
dsn: __SENTRY_DSN__,
...pluginParams,
integrations,
});

window.Sentry = Sentry;
Expand Down
4 changes: 3 additions & 1 deletion packages/gatsby/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,10 @@
"gatsby": "^2.0.0 || ^3.0.0"
},
"devDependencies": {
"@sentry-internal/eslint-config-sdk": "6.13.3",
"@sentry-internal/eslint-config-sdk": "6.13.2",
"@sentry/types": "6.13.2",
"@testing-library/react": "^10.4.9",
"@types/node": "^16.10.3",
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need node types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Linter was complaining about process, used in defaultOptions. Since they'll be removed, I can remove this dev dep too.

"jest": "^24.7.1",
"npm-run-all": "^4.1.2",
"prettier": "1.19.0",
Expand Down
2 changes: 2 additions & 0 deletions packages/gatsby/src/index.ts
Original file line number Diff line number Diff line change
@@ -1 +1,3 @@
export * from '@sentry/react';

export { init } from './sdk';
33 changes: 33 additions & 0 deletions packages/gatsby/src/sdk.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import { init as reactInit, SDK_VERSION } from '@sentry/react';

import { getIntegrationsFromOptions } from './utils/integrations';
import { GatsbyOptions } from './utils/types';

export const defaultOptions = {
autoSessionTracking: true,
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 on by default now (since v6), we can rm -rf this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I want the behavior to be as close as possible to the state prior to this refactoring. I'll make another PR to remove these options.

environment: process.env.NODE_ENV || 'development',
Copy link
Member

Choose a reason for hiding this comment

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

Tbh, should just be undefined by default

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same answer as previous.

};

/**
* Inits the Sentry Gatsby SDK.
*/
export function init(options: GatsbyOptions): void {
options._metadata = options._metadata || {};
options._metadata.sdk = options._metadata.sdk || {
name: 'sentry.javascript.gatsby',
packages: [
{
name: 'npm:@sentry/gatsby',
version: SDK_VERSION,
},
],
version: SDK_VERSION,
};

const integrations = getIntegrationsFromOptions(options);
Copy link
Member

Choose a reason for hiding this comment

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

nit: This function can just be in the same file for now, it's just added overhead to keep it in a separate file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you mean gatsby-browser, no. If we want to provide users the option to initialize the SDK from a place without gatsby-browser, this code must live in the SDK itself. Of course, this isn't required at the moment, but without that goal in mind, the whole PR isn't required at all. Moving it here is a refactoring, included it in this PR (and not in the feature PR that will follow-up) to make it easier to review and identify bugs.

Copy link
Member

Choose a reason for hiding this comment

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

No meaning getIntegrationsFromOptions from packages/gatsby/src/utils/integrations.ts can just be a part of packages/gatsby/src/sdk.ts. Agree with the change from gatsby-browser

reactInit({
...defaultOptions,
...options,
integrations,
});
}
21 changes: 21 additions & 0 deletions packages/gatsby/src/utils/integrations.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
import * as Tracing from '@sentry/tracing';
import { Integration } from '@sentry/types';

import { GatsbyOptions } from './types';

/**
* Returns the integrations to add to the SDK.
* If tracing is enabled, `BrowserTracing` is always present.
*
* @param options The options users have defined.
*/
export function getIntegrationsFromOptions(options: GatsbyOptions): Integration[] {
const integrations = [...(options.integrations || [])];
Copy link
Member

Choose a reason for hiding this comment

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

integrations can be a function as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Functions are non-serializable. The end goal is to provide a way to define these options. This refactoring is the previous step to that feature, which only supports non-serializable options. So no, at the moment they can't be functions.

if (
Tracing.hasTracingEnabled(options) &&
!integrations.some(integration => integration.name === Tracing.Integrations.BrowserTracing.name)
) {
integrations.push(new Tracing.Integrations.BrowserTracing());
}
return integrations;
}
3 changes: 3 additions & 0 deletions packages/gatsby/src/utils/types.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { BrowserOptions } from '@sentry/react';

export type GatsbyOptions = BrowserOptions;
60 changes: 11 additions & 49 deletions packages/gatsby/test/gatsby-browser.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@ const { onClientEntry } = require('../gatsby-browser');
(global as any).__SENTRY_DSN__ = 'https://[email protected]/0';

let sentryInit = jest.fn();
jest.mock('@sentry/react', () => {
const original = jest.requireActual('@sentry/react');
jest.mock('@sentry/gatsby', () => {
const original = jest.requireActual('@sentry/gatsby');
return {
...original,
init: (...args: any[]) => {
Expand Down Expand Up @@ -38,42 +38,23 @@ describe('onClientEntry', () => {
(window as any).Sentry = undefined;
});

it('inits Sentry by default', () => {
onClientEntry(undefined, {});
it.each([
[{}, ['dsn', 'release']],
[{ key: 'value' }, ['dsn', 'release', 'key']],
])('inits Sentry by default', (pluginParams, expectedKeys) => {
onClientEntry(undefined, pluginParams);
expect(sentryInit).toHaveBeenCalledTimes(1);
expect(sentryInit).toHaveBeenLastCalledWith({
dsn: (global as any).__SENTRY_DSN__,
environment: process.env.NODE_ENV,
integrations: [],
release: (global as any).__SENTRY_RELEASE__,
autoSessionTracking: true,
_metadata: {
sdk: {
name: 'sentry.javascript.gatsby',
packages: [
{
name: 'npm:@sentry/gatsby',
version: expect.any(String),
},
],
version: expect.any(String),
},
},
});
const calledWith = sentryInit.mock.calls[0][0];
for (const key of expectedKeys) {
expect(calledWith[key]).toBeDefined();
}
});

it('sets window.Sentry', () => {
onClientEntry(undefined, {});
expect((window as any).Sentry).not.toBeUndefined();
});

it('adds Tracing extension methods', () => {
onClientEntry(undefined, {});

expect(tracingAddExtensionMethods).toHaveBeenCalledTimes(1);
expect(tracingAddExtensionMethods).toHaveBeenLastCalledWith();
});

it('sets a tracesSampleRate if defined as option', () => {
onClientEntry(undefined, { tracesSampleRate: 0.5 });
expect(sentryInit).toHaveBeenLastCalledWith(
Expand All @@ -93,25 +74,6 @@ describe('onClientEntry', () => {
);
});

it('adds `BrowserTracing` integration if tracesSampleRate is defined', () => {
onClientEntry(undefined, { tracesSampleRate: 0.5 });
expect(sentryInit).toHaveBeenLastCalledWith(
expect.objectContaining({
integrations: [expect.objectContaining({ name: 'BrowserTracing' })],
}),
);
});

it('adds `BrowserTracing` integration if tracesSampler is defined', () => {
const tracesSampler = jest.fn();
onClientEntry(undefined, { tracesSampler });
expect(sentryInit).toHaveBeenLastCalledWith(
expect.objectContaining({
integrations: [expect.objectContaining({ name: 'BrowserTracing' })],
}),
);
});

it('only defines a single `BrowserTracing` integration', () => {
const Tracing = jest.requireActual('@sentry/tracing');
const integrations = [new Tracing.Integrations.BrowserTracing()];
Expand Down
2 changes: 1 addition & 1 deletion packages/gatsby/test/index.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import * as GatsbyIntegration from '../src';
import * as GatsbyIntegration from '../src/index';

describe('package', () => {
it('exports init', () => {
Expand Down
92 changes: 92 additions & 0 deletions packages/gatsby/test/sdk.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
import { init as reactInitRaw, SDK_VERSION } from '@sentry/react';
import { Integrations } from '@sentry/tracing';
import { Integration, Package } from '@sentry/types';

import { defaultOptions, init as gatsbyInit } from '../src/sdk';
import { GatsbyOptions } from '../src/utils/types';

const reactInit = reactInitRaw as jest.Mock;
jest.mock('@sentry/react');

describe('Initialize React SDK', () => {
afterEach(() => reactInit.mockReset());

test('Default init props', () => {
gatsbyInit({});
expect(reactInit).toHaveBeenCalledTimes(1);
const calledWith = reactInit.mock.calls[0][0];
expect(calledWith).toMatchObject(defaultOptions);
});

test('Has correct SDK metadata', () => {
gatsbyInit({});
const calledWith = reactInit.mock.calls[0][0];
const sdkMetadata = calledWith._metadata.sdk;
expect(sdkMetadata.name).toStrictEqual('sentry.javascript.gatsby');
expect(sdkMetadata.version).toBe(SDK_VERSION);
expect(sdkMetadata.packages).toMatchInlineSnapshot(`
Array [
Object {
"name": "npm:@sentry/gatsby",
"version": "6.13.3",
},
]
`);
});

describe('Environment', () => {
test('process.env', () => {
gatsbyInit({});
expect(reactInit).toHaveBeenCalledTimes(1);
const callingObject = reactInit.mock.calls[0][0];
expect(callingObject.environment).toStrictEqual('test');
});

test('defined in the options', () => {
gatsbyInit({
environment: 'custom env!',
});
expect(reactInit).toHaveBeenCalledTimes(1);
const callingObject = reactInit.mock.calls[0][0];
expect(callingObject.environment).toStrictEqual('custom env!');
});
});

test('Has BrowserTracing if tracing enabled', () => {
gatsbyInit({ tracesSampleRate: 1 });
expect(reactInit).toHaveBeenCalledTimes(1);
const calledWith = reactInit.mock.calls[0][0];
const integrationNames: string[] = calledWith.integrations.map((integration: Integration) => integration.name);
expect(integrationNames.some(name => name === 'BrowserTracing')).toBe(true);
});
});

describe('Integrations from options', () => {
afterEach(() => reactInit.mockClear());

test.each([
['tracing disabled, no integrations', {}, []],
['tracing enabled, no integrations', { tracesSampleRate: 1 }, ['BrowserTracing']],
[
'tracing disabled, with Integrations.BrowserTracing',
{ integrations: [new Integrations.BrowserTracing()] },
['BrowserTracing'],
],
[
'tracing enabled, with Integrations.BrowserTracing',
{ tracesSampleRate: 1, integrations: [new Integrations.BrowserTracing()] },
['BrowserTracing'],
],
[
'tracing enabled, with another integration',
{ tracesSampleRate: 1, integrations: [new Integrations.Express()] },
['Express', 'BrowserTracing'],
],
['tracing disabled, with another integration', { integrations: [new Integrations.Express()] }, ['Express']],
])('%s', (_testName, options: GatsbyOptions, expectedIntNames: string[]) => {
gatsbyInit(options);
const integrations: Integration[] = reactInit.mock.calls[0][0].integrations;
expect(integrations).toHaveLength(expectedIntNames.length);
integrations.map((integration, idx) => expect(integration.name).toStrictEqual(expectedIntNames[idx]));
});
});
Loading