-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(sveltekit): Auto-wrap load
functions with proxy module
#7994
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
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 |
---|---|---|
@@ -0,0 +1,123 @@ | ||
/* eslint-disable @sentry-internal/sdk/no-optional-chaining */ | ||
import * as fs from 'fs'; | ||
import * as path from 'path'; | ||
import type { Plugin } from 'vite'; | ||
|
||
export const WRAPPED_MODULE_SUFFIX = '?sentry-auto-wrap'; | ||
|
||
export type AutoInstrumentSelection = { | ||
/** | ||
* If this flag is `true`, the Sentry plugins will automatically instrument the `load` function of | ||
* your universal `load` functions declared in your `+page.(js|ts)` and `+layout.(js|ts)` files. | ||
* | ||
* @default true | ||
*/ | ||
load?: boolean; | ||
|
||
/** | ||
* If this flag is `true`, the Sentry plugins will automatically instrument the `load` function of | ||
* your server-only `load` functions declared in your `+page.server.(js|ts)` | ||
* and `+layout.server.(js|ts)` files. | ||
* | ||
* @default true | ||
*/ | ||
serverLoad?: boolean; | ||
}; | ||
|
||
type AutoInstrumentPluginOptions = AutoInstrumentSelection & { | ||
debug: boolean; | ||
}; | ||
|
||
/** | ||
* Creates a Vite plugin that automatically instruments the parts of the app | ||
* specified in @param options. This includes | ||
* | ||
* - universal `load` functions from `+page.(js|ts)` and `+layout.(js|ts)` files | ||
* - server-only `load` functions from `+page.server.(js|ts)` and `+layout.server.(js|ts)` files | ||
* | ||
* @returns the plugin | ||
*/ | ||
export function makeAutoInstrumentationPlugin(options: AutoInstrumentPluginOptions): Plugin { | ||
const { load: shouldWrapLoad, serverLoad: shouldWrapServerLoad, debug } = options; | ||
|
||
return { | ||
name: 'sentry-auto-instrumentation', | ||
// This plugin needs to run as early as possible, before the SvelteKit plugin virtualizes all paths and ids | ||
enforce: 'pre', | ||
|
||
async load(id) { | ||
const applyUniversalLoadWrapper = | ||
shouldWrapLoad && | ||
/^\+(page|layout)\.(js|ts|mjs|mts)$/.test(path.basename(id)) && | ||
(await canWrapLoad(id, debug)); | ||
|
||
if (applyUniversalLoadWrapper) { | ||
// eslint-disable-next-line no-console | ||
debug && console.log(`Wrapping ${id} with Sentry load wrapper`); | ||
return getWrapperCode('wrapLoadWithSentry', `${id}${WRAPPED_MODULE_SUFFIX}`); | ||
} | ||
|
||
const applyServerLoadWrapper = | ||
shouldWrapServerLoad && | ||
/^\+(page|layout)\.server\.(js|ts|mjs|mts)$/.test(path.basename(id)) && | ||
(await canWrapLoad(id, debug)); | ||
|
||
if (applyServerLoadWrapper) { | ||
// eslint-disable-next-line no-console | ||
debug && console.log(`Wrapping ${id} with Sentry load wrapper`); | ||
return getWrapperCode('wrapServerLoadWithSentry', `${id}${WRAPPED_MODULE_SUFFIX}`); | ||
} | ||
|
||
return null; | ||
}, | ||
}; | ||
} | ||
|
||
/** | ||
* We only want to apply our wrapper to files that | ||
* | ||
* - Have no Sentry code yet in them. This is to avoid double-wrapping or interferance with custom | ||
* Sentry calls. | ||
* - Actually declare a `load` function. The second check of course is not 100% accurate, but it's good enough. | ||
* Injecting our wrapper into files that don't declare a `load` function would result in a build-time warning | ||
* because vite/rollup warns if we reference an export from the user's file in our wrapping code that | ||
* doesn't exist. | ||
* | ||
* Exported for testing | ||
* | ||
* @returns `true` if we can wrap the given file, `false` otherwise | ||
*/ | ||
export async function canWrapLoad(id: string, debug: boolean): Promise<boolean> { | ||
const code = (await fs.promises.readFile(id, 'utf8')).toString(); | ||
|
||
const codeWithoutComments = code.replace(/(\/\/.*| ?\/\*[^]*?\*\/)(,?)$/gm, ''); | ||
|
||
const hasSentryContent = codeWithoutComments.includes('@sentry/sveltekit'); | ||
if (hasSentryContent) { | ||
// eslint-disable-next-line no-console | ||
debug && console.log(`Skipping wrapping ${id} because it already contains Sentry code`); | ||
} | ||
|
||
const hasLoadDeclaration = /((const|let|var|function)\s+load\s*(=|\())|as\s+load\s*(,|})/gm.test(codeWithoutComments); | ||
if (!hasLoadDeclaration) { | ||
// eslint-disable-next-line no-console | ||
debug && console.log(`Skipping wrapping ${id} because it doesn't declare a \`load\` function`); | ||
} | ||
|
||
return !hasSentryContent && hasLoadDeclaration; | ||
} | ||
|
||
/** | ||
* Return wrapper code fo the given module id and wrapping function | ||
*/ | ||
function getWrapperCode( | ||
wrapperFunction: 'wrapLoadWithSentry' | 'wrapServerLoadWithSentry', | ||
idWithSuffix: string, | ||
): string { | ||
return ( | ||
`import { ${wrapperFunction} } from "@sentry/sveltekit";` + | ||
`import * as userModule from ${JSON.stringify(idWithSuffix)};` + | ||
`export const load = userModule.load ? ${wrapperFunction}(userModule.load) : undefined;` + | ||
`export * from ${JSON.stringify(idWithSuffix)};` | ||
); | ||
} | ||
Comment on lines
+113
to
+123
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. Honestly I really like that we just do template strings for this instead of typescript templates. Makes this so much simpler. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,14 @@ | ||
import type { SentryVitePluginOptions } from '@sentry/vite-plugin'; | ||
import type { Plugin } from 'vite'; | ||
|
||
import type { AutoInstrumentSelection } from './autoInstrument'; | ||
import { makeAutoInstrumentationPlugin } from './autoInstrument'; | ||
import { makeCustomSentryVitePlugin } from './sourceMaps'; | ||
|
||
type SourceMapsUploadOptions = { | ||
/** | ||
* If this flag is `true`, the Sentry plugins will automatically upload source maps to Sentry. | ||
* Defaults to `true`. | ||
* @default true`. | ||
*/ | ||
autoUploadSourceMaps?: boolean; | ||
|
||
|
@@ -17,16 +19,32 @@ type SourceMapsUploadOptions = { | |
sourceMapsUploadOptions?: Partial<SentryVitePluginOptions>; | ||
}; | ||
|
||
type AutoInstrumentOptions = { | ||
/** | ||
* The Sentry plugin will automatically instrument certain parts of your SvelteKit application at build time. | ||
* Set this option to `false` to disable this behavior or what is instrumentated by passing an object. | ||
* | ||
* Auto instrumentation includes: | ||
* - Universal `load` functions in `+page.(js|ts)` files | ||
* - Server-only `load` functions in `+page.server.(js|ts)` files | ||
* | ||
* @default true (meaning, the plugin will instrument all of the above) | ||
*/ | ||
autoInstrument?: boolean | AutoInstrumentSelection; | ||
}; | ||
|
||
export type SentrySvelteKitPluginOptions = { | ||
/** | ||
* If this flag is `true`, the Sentry plugins will log some useful debug information. | ||
* Defaults to `false`. | ||
* @default false. | ||
*/ | ||
debug?: boolean; | ||
} & SourceMapsUploadOptions; | ||
} & SourceMapsUploadOptions & | ||
AutoInstrumentOptions; | ||
|
||
const DEFAULT_PLUGIN_OPTIONS: SentrySvelteKitPluginOptions = { | ||
autoUploadSourceMaps: true, | ||
autoInstrument: 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. Doesn't have to be now but we should think about adding an escape hatch for not auto instrumenting particular routes. I've seen users in nexts that found this useful (eg some people didn't want to instrument their auth routes). 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. Yes, good point. As discussed, I'll do this in a follow-up PR but I agree that it's also a great way of giving people a quick way to opt out of auto instrumenting in case they get an error. |
||
debug: false, | ||
}; | ||
|
||
|
@@ -43,7 +61,22 @@ export async function sentrySvelteKit(options: SentrySvelteKitPluginOptions = {} | |
...options, | ||
}; | ||
|
||
const sentryPlugins = []; | ||
const sentryPlugins: Plugin[] = []; | ||
|
||
if (mergedOptions.autoInstrument) { | ||
const pluginOptions: AutoInstrumentSelection = { | ||
load: true, | ||
serverLoad: true, | ||
...(typeof mergedOptions.autoInstrument === 'object' ? mergedOptions.autoInstrument : {}), | ||
}; | ||
|
||
sentryPlugins.push( | ||
makeAutoInstrumentationPlugin({ | ||
...pluginOptions, | ||
debug: options.debug || false, | ||
}), | ||
); | ||
} | ||
|
||
if (mergedOptions.autoUploadSourceMaps) { | ||
const pluginOptions = { | ||
|
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.
I am very curious how this will work out. I think this is a very sane approach 👍
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.
Yeah me too, I guess we'll learn it soon enough 😅