minor: support @sentry/sveltekit routing instrumentation #98
Conversation
ghost
enabled auto-merge
WalkthroughThe changes primarily focus on enhancing the Sentry SDK integration for Svelte applications. They introduce new functions to handle client integrations, apply SDK metadata, and manage the Changes
TipsChat with CodeRabbit Bot (
|
Deploying with
|
| Latest commit: |
261b748
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a0489762.sentry-sveltekit-cloudflare.pages.dev |
| Branch Preview URL: | https://issues-97.sentry-sveltekit-cloudflare.pages.dev |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Files ignored due to filter (2)
- package.json
- pnpm-lock.yaml
Files selected for processing (8)
- packages/lib/scripts/build.ts (1 hunks)
- packages/lib/src/client/addClientIntegrations.ts (1 hunks)
- packages/lib/src/client/applySdkMetadata.ts (1 hunks)
- packages/lib/src/client/init.ts (2 hunks)
- packages/lib/src/client/restoreFetch.ts (1 hunks)
- packages/lib/src/client/router.ts (1 hunks)
- packages/lib/src/client/sentryInit.ts (1 hunks)
- packages/lib/src/client/switchToFetchProxy.ts (1 hunks)
Files skipped from review due to trivial changes (3)
- packages/lib/scripts/build.ts
- packages/lib/src/client/init.ts
- packages/lib/src/client/router.ts
Additional comments (Suppressed): 1
packages/lib/src/client/restoreFetch.ts (1)
- 1-11: The
restoreFetchfunction is replacing the globalfetchfunction with the originalfetchfunction passed as an argument. This is a good practice to restore the original behavior after the Sentry SDK initialization. However, it's not clear why thefetchfunction is being stored in_sentryFetchProxyon line 7. If this is not used elsewhere, it might be unnecessary. Please verify.- globalWithSentryFetchProxy._sentryFetchProxy = - globalWithSentryFetchProxy.fetch - globalWithSentryFetchProxy.fetch = actualFetch
| import { hasTracingEnabled } from '@sentry/core' | ||
| import { BrowserOptions, BrowserTracing } from '@sentry/svelte' | ||
| import { addOrUpdateIntegration } from '@sentry/utils' | ||
| import { svelteKitRoutingInstrumentation } from './router.js' | ||
|
|
||
| export const addClientIntegrations = (options: BrowserOptions) => { | ||
| let integrations = options.integrations || [] | ||
|
|
||
| if (hasTracingEnabled(options)) { | ||
| const defaultBrowserTracingIntegration = new BrowserTracing({ | ||
| routingInstrumentation: svelteKitRoutingInstrumentation | ||
| }) | ||
|
|
||
| integrations = addOrUpdateIntegration( | ||
| defaultBrowserTracingIntegration, | ||
| integrations, | ||
| { | ||
| 'options.routingInstrumentation': svelteKitRoutingInstrumentation | ||
| } | ||
| ) | ||
| } | ||
|
|
||
| options.integrations = integrations | ||
| } |
There was a problem hiding this comment.
The function addClientIntegrations modifies the options object passed to it. This can lead to unexpected side effects if the caller is not aware of this behavior. Consider returning a new object with the updated integrations instead of modifying the input object. This would make the function pure and easier to reason about.
- options.integrations = integrations
+ return { ...options, integrations };Then, the caller can decide whether to replace the original options object or not.
| import { WINDOW } from '@sentry/svelte' | ||
|
|
||
| export function switchToFetchProxy() { | ||
| const globalWithSentryFetchProxy = WINDOW | ||
|
|
||
| const actualFetch = globalWithSentryFetchProxy.fetch | ||
|
|
||
| // @ts-expect-error TODO: fix this | ||
| if (globalWithSentryFetchProxy._sentryFetchProxy && actualFetch) { | ||
| globalWithSentryFetchProxy.fetch = | ||
| // @ts-expect-error TODO: fix this | ||
| globalWithSentryFetchProxy._sentryFetchProxy | ||
| return actualFetch | ||
| } | ||
|
|
||
| return undefined | ||
| } |
There was a problem hiding this comment.
The function switchToFetchProxy switches the global fetch function to a Sentry proxy. However, it doesn't check if the _sentryFetchProxy is a function before assigning it to fetch. This could potentially break the application if _sentryFetchProxy is not a function. Consider adding a type check before the assignment.
- if (globalWithSentryFetchProxy._sentryFetchProxy && actualFetch) {
+ if (typeof globalWithSentryFetchProxy._sentryFetchProxy === 'function' && actualFetch) {Also, the function returns undefined if the conditions are not met. This could lead to unexpected behavior in the calling code. Consider throwing an error or returning a default value instead.
| import { BrowserOptions, configureScope, init } from '@sentry/svelte' | ||
| import { addClientIntegrations } from './addClientIntegrations.js' | ||
| import { applySdkMetadata } from './applySdkMetadata.js' | ||
| import { restoreFetch } from './restoreFetch.js' | ||
| import { switchToFetchProxy } from './switchToFetchProxy.js' | ||
|
|
||
| export const sentryInit = (options: BrowserOptions) => { | ||
| applySdkMetadata(options, ['sveltekit', 'svelte']) | ||
|
|
||
| addClientIntegrations(options) | ||
|
|
||
| // 1. Switch window.fetch to our fetch proxy we injected earlier | ||
| const actualFetch = switchToFetchProxy() | ||
|
|
||
| // 2. Initialize the SDK which will instrument our proxy | ||
| init(options) | ||
|
|
||
| // 3. Restore the original fetch now that our proxy is instrumented | ||
| if (actualFetch) { | ||
| restoreFetch(actualFetch) | ||
| } | ||
|
|
||
| configureScope((scope) => { | ||
| scope.setTag('runtime', 'browser') | ||
| }) | ||
| } |
There was a problem hiding this comment.
The sentryInit function is well-structured and follows a clear sequence of steps for initializing Sentry with SvelteKit. However, there is a potential issue with the restoreFetch function. If switchToFetchProxy fails and returns null or undefined, restoreFetch will be called with an undefined argument. This could lead to unexpected behavior or errors. Consider adding error handling or a check to ensure actualFetch is a function before restoring it.
// 3. Restore the original fetch now that our proxy is instrumented
if (actualFetch && typeof actualFetch === 'function') {
restoreFetch(actualFetch)
}| import { SDK_VERSION } from '@sentry/core' | ||
| import { BrowserOptions } from '@sentry/svelte' | ||
|
|
||
| const PACKAGE_NAME_PREFIX = 'npm:@sentry/' | ||
|
|
||
| export const applySdkMetadata = (options: BrowserOptions, names: string[]) => { | ||
| options._metadata = options._metadata || {} | ||
| options._metadata.sdk = options._metadata.sdk || { | ||
| name: 'sentry.javascript.sveltekit', | ||
| packages: names.map((name) => ({ | ||
| name: `${PACKAGE_NAME_PREFIX}${name}`, | ||
| version: SDK_VERSION | ||
| })), | ||
| version: SDK_VERSION | ||
| } | ||
| } |
There was a problem hiding this comment.
The function applySdkMetadata modifies the options object passed to it. This is a side effect that could lead to unexpected behavior if the caller is not aware of it. Consider returning a new object instead of modifying the existing one. This would make the function pure and easier to reason about.
- export const applySdkMetadata = (options: BrowserOptions, names: string[]) => {
- options._metadata = options._metadata || {}
- options._metadata.sdk = options._metadata.sdk || {
- name: 'sentry.javascript.sveltekit',
- packages: names.map((name) => ({
- name: `${PACKAGE_NAME_PREFIX}${name}`,
- version: SDK_VERSION
- })),
- version: SDK_VERSION
- }
- }
+ export const applySdkMetadata = (options: BrowserOptions, names: string[]): BrowserOptions => {
+ const newOptions = { ...options };
+ newOptions._metadata = newOptions._metadata || {}
+ newOptions._metadata.sdk = newOptions._metadata.sdk || {
+ name: 'sentry.javascript.sveltekit',
+ packages: names.map((name) => ({
+ name: `${PACKAGE_NAME_PREFIX}${name}`,
+ version: SDK_VERSION
+ })),
+ version: SDK_VERSION
+ }
+ return newOptions;
+ }
close #97
Summary by CodeRabbit
$app/storesas an external dependency, improving the overall build process.BrowserOptionsobject with new functions for adding client integrations and applying SDK metadata. This will provide more customization options and better tracking of SDK usage.fetchfunction to a proxy, improving the handling of network requests.Please note, there are a couple of TypeScript errors that need to be addressed in the new
switchToFetchProxyfunction.