-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Changes from all commits
5b77f98
c32f731
dd5e4ba
8e8ce8b
6387b9d
4661210
7c757c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,3 @@ | ||
export * from '@sentry/react'; | ||
|
||
export { init } from './sdk'; |
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, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be on by default now (since v6), we can There was a problem hiding this comment. Choose a reason for hiding this commentThe 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', | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tbh, should just be There was a problem hiding this comment. Choose a reason for hiding this commentThe 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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you mean There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No meaning |
||
reactInit({ | ||
...defaultOptions, | ||
...options, | ||
integrations, | ||
}); | ||
} |
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 || [])]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. integrations can be a function as well There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
import { BrowserOptions } from '@sentry/react'; | ||
|
||
export type GatsbyOptions = BrowserOptions; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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[]) => { | ||
|
@@ -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( | ||
|
@@ -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()]; | ||
|
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])); | ||
}); | ||
}); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 indefaultOptions
. Since they'll be removed, I can remove this dev dep too.