From 72003dd4eaf1bb9f4572db80d5b8ee55f6c80167 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 25 Apr 2023 15:02:36 +0200 Subject: [PATCH 1/4] feat(sveltekit): Auto-wrap `load` functions with proxy module --- packages/sveltekit/package.json | 1 - packages/sveltekit/src/server/utils.ts | 8 + packages/sveltekit/src/vite/autoInstrument.ts | 123 +++++++++++ .../sveltekit/src/vite/sentryVitePlugins.ts | 41 +++- packages/sveltekit/src/vite/sourceMaps.ts | 21 +- .../test/vite/autoInstrument.test.ts | 204 ++++++++++++++++++ .../test/vite/sentrySvelteKitPlugins.test.ts | 57 ++++- .../sveltekit/test/vite/sourceMaps.test.ts | 2 +- 8 files changed, 442 insertions(+), 15 deletions(-) create mode 100644 packages/sveltekit/src/vite/autoInstrument.ts create mode 100644 packages/sveltekit/test/vite/autoInstrument.test.ts diff --git a/packages/sveltekit/package.json b/packages/sveltekit/package.json index 577cf502d789..6ce6b27ef566 100644 --- a/packages/sveltekit/package.json +++ b/packages/sveltekit/package.json @@ -28,7 +28,6 @@ "@sentry/types": "7.49.0", "@sentry/utils": "7.49.0", "@sentry/vite-plugin": "^0.6.0", - "magic-string": "^0.30.0", "sorcery": "0.11.0" }, "devDependencies": { diff --git a/packages/sveltekit/src/server/utils.ts b/packages/sveltekit/src/server/utils.ts index 644cd8477fff..17fc855ebc16 100644 --- a/packages/sveltekit/src/server/utils.ts +++ b/packages/sveltekit/src/server/utils.ts @@ -2,6 +2,8 @@ import type { DynamicSamplingContext, StackFrame, TraceparentData } from '@sentr import { baggageHeaderToDynamicSamplingContext, basename, extractTraceparentData } from '@sentry/utils'; import type { RequestEvent } from '@sveltejs/kit'; +import { WRAPPED_MODULE_SUFFIX } from '../vite/autoInstrument'; + /** * Takes a request event and extracts traceparent and DSC data * from the `sentry-trace` and `baggage` DSC headers. @@ -52,5 +54,11 @@ export function rewriteFramesIteratee(frame: StackFrame): StackFrame { delete frame.module; + // In dev-mode, the WRAPPED_MODULE_SUFFIX is still present in the frame's file name. + // We need to remove it to make sure that the frame's filename matches the actual file + if (frame.filename.endsWith(WRAPPED_MODULE_SUFFIX)) { + frame.filename = frame.filename.slice(0, -WRAPPED_MODULE_SUFFIX.length); + } + return frame; } diff --git a/packages/sveltekit/src/vite/autoInstrument.ts b/packages/sveltekit/src/vite/autoInstrument.ts new file mode 100644 index 000000000000..4150e873c394 --- /dev/null +++ b/packages/sveltekit/src/vite/autoInstrument.ts @@ -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 async function makeAutoInstrumentationPlugin(options: AutoInstrumentPluginOptions): Promise { + 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 { + 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 wrappiung ${id} because it already contains Sentry code`); + } + + const hasLoadDeclaration = /(const|let|var|function)\s+load\s*(=|\()/gm.test(codeWithoutComments); + if (!hasLoadDeclaration) { + // eslint-disable-next-line no-console + debug && console.log(`Skipping wrappiung ${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)};` + ); +} diff --git a/packages/sveltekit/src/vite/sentryVitePlugins.ts b/packages/sveltekit/src/vite/sentryVitePlugins.ts index 6c932d2f3728..615112744f81 100644 --- a/packages/sveltekit/src/vite/sentryVitePlugins.ts +++ b/packages/sveltekit/src/vite/sentryVitePlugins.ts @@ -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; }; +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, 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( + await makeAutoInstrumentationPlugin({ + ...pluginOptions, + debug: options.debug || false, + }), + ); + } if (mergedOptions.autoUploadSourceMaps) { const pluginOptions = { diff --git a/packages/sveltekit/src/vite/sourceMaps.ts b/packages/sveltekit/src/vite/sourceMaps.ts index 88a4c3424719..8c7662a93813 100644 --- a/packages/sveltekit/src/vite/sourceMaps.ts +++ b/packages/sveltekit/src/vite/sourceMaps.ts @@ -1,5 +1,5 @@ import { getSentryRelease } from '@sentry/node'; -import { uuid4 } from '@sentry/utils'; +import { escapeStringForRegex, uuid4 } from '@sentry/utils'; import type { SentryVitePluginOptions } from '@sentry/vite-plugin'; import { sentryVitePlugin } from '@sentry/vite-plugin'; import * as child_process from 'child_process'; @@ -10,6 +10,7 @@ import * as path from 'path'; import * as sorcery from 'sorcery'; import type { Plugin } from 'vite'; +import { WRAPPED_MODULE_SUFFIX } from './autoInstrument'; import { getAdapterOutputDir, loadSvelteConfig } from './svelteConfig'; // sorcery has no types, so these are some basic type definitions: @@ -74,9 +75,9 @@ export async function makeCustomSentryVitePlugin(options?: SentryVitePluginOptio let isSSRBuild = true; const customPlugin: Plugin = { - name: 'sentry-vite-plugin-custom', + name: 'sentry-upload-source-maps', apply: 'build', // only apply this plugin at build time - enforce: 'post', + enforce: 'post', // this needs to be set to post, otherwise we don't pick up the output from the SvelteKit adapter // These hooks are copied from the original Sentry Vite plugin. // They're mostly responsible for options parsing and release injection. @@ -117,6 +118,8 @@ export async function makeCustomSentryVitePlugin(options?: SentryVitePluginOptio } const outDir = path.resolve(process.cwd(), outputDir); + // eslint-disable-next-line no-console + debug && console.log('[Source Maps Plugin] Looking up source maps in', outDir); const jsFiles = getFiles(outDir).filter(file => file.endsWith('.js')); // eslint-disable-next-line no-console @@ -145,6 +148,18 @@ export async function makeCustomSentryVitePlugin(options?: SentryVitePluginOptio console.error('[Source Maps Plugin] error while flattening', file, e); } } + + // We need to remove the query string from the source map files that our auto-instrument plugin added + // to proxy the load functions during building. + const mapFile = `${file}.map`; + if (fs.existsSync(mapFile)) { + const mapContent = (await fs.promises.readFile(mapFile, 'utf-8')).toString(); + const cleanedMapContent = mapContent.replace( + new RegExp(escapeStringForRegex(WRAPPED_MODULE_SUFFIX), 'gm'), + '', + ); + await fs.promises.writeFile(mapFile, cleanedMapContent); + } }); try { diff --git a/packages/sveltekit/test/vite/autoInstrument.test.ts b/packages/sveltekit/test/vite/autoInstrument.test.ts new file mode 100644 index 000000000000..421835ca35be --- /dev/null +++ b/packages/sveltekit/test/vite/autoInstrument.test.ts @@ -0,0 +1,204 @@ +import { vi } from 'vitest'; + +// import { makeAutoInstrumentationPlugin } from '../../src/vite/autoInstrument'; +import * as autoInstument from '../../src/vite/autoInstrument'; + +const DEFAULT_CONTENT = ` + export const load = () => {}; + export const prerender = true; +`; + +let fileContent: string | undefined; + +vi.mock('fs', async () => { + const actual = await vi.importActual('fs'); + return { + // @ts-ignore this exists, I promise! + ...actual, + promises: { + // @ts-ignore this also exists, I promise! + ...actual.promises, + readFile: vi.fn().mockImplementation(() => { + return fileContent || DEFAULT_CONTENT; + }), + }, + }; +}); + +vi.mock('rollup', () => { + return { + rollup: vi.fn().mockReturnValue({ generate: vi.fn().mockReturnValue({ output: ['transformed'] }) }), + }; +}); + +describe('makeAutoInstrumentationPlugin()', () => { + beforeEach(() => { + vi.clearAllMocks(); + fileContent = undefined; + }); + + it('returns the auto instrumentation plugin', async () => { + const plugin = await autoInstument.makeAutoInstrumentationPlugin({ debug: true, load: true, serverLoad: true }); + expect(plugin.name).toEqual('sentry-auto-instrumentation'); + expect(plugin.enforce).toEqual('pre'); + expect(plugin.load).toEqual(expect.any(Function)); + }); + + describe.each([ + 'path/to/+page.ts', + 'path/to/+page.js', + 'path/to/+page.mts', + 'path/to/+page.mjs', + 'path/to/+layout.ts', + 'path/to/+layout.js', + 'path/to/+layout.mts', + 'path/to/+layout.mjs', + ])('transform %s files', (path: string) => { + it('wraps universal load if `load` option is `true`', async () => { + const plugin = await autoInstument.makeAutoInstrumentationPlugin({ debug: false, load: true, serverLoad: true }); + // @ts-ignore this exists + const loadResult = await plugin.load(path); + expect(loadResult).toEqual( + 'import { wrapLoadWithSentry } from "@sentry/sveltekit";' + + `import * as userModule from "${path}?sentry-auto-wrap";` + + 'export const load = userModule.load ? wrapLoadWithSentry(userModule.load) : undefined;' + + `export * from "${path}?sentry-auto-wrap";`, + ); + }); + + it("doesn't wrap universal load if `load` option is `false`", async () => { + const plugin = await autoInstument.makeAutoInstrumentationPlugin({ + debug: false, + load: false, + serverLoad: false, + }); + // @ts-ignore this exists + const loadResult = await plugin.load(path); + expect(loadResult).toEqual(null); + }); + }); + + describe.each([ + 'path/to/+page.server.ts', + 'path/to/+page.server.js', + 'path/to/+page.server.mts', + 'path/to/+page.server.mjs', + 'path/to/+layout.server.ts', + 'path/to/+layout.server.js', + 'path/to/+layout.server.mts', + 'path/to/+layout.server.mjs', + ])('transform %s files', (path: string) => { + it('wraps universal load if `load` option is `true`', async () => { + const plugin = await autoInstument.makeAutoInstrumentationPlugin({ debug: false, load: false, serverLoad: true }); + // @ts-ignore this exists + const loadResult = await plugin.load(path); + expect(loadResult).toEqual( + 'import { wrapServerLoadWithSentry } from "@sentry/sveltekit";' + + `import * as userModule from "${path}?sentry-auto-wrap";` + + 'export const load = userModule.load ? wrapServerLoadWithSentry(userModule.load) : undefined;' + + `export * from "${path}?sentry-auto-wrap";`, + ); + }); + + it("doesn't wrap universal load if `load` option is `false`", async () => { + const plugin = await autoInstument.makeAutoInstrumentationPlugin({ + debug: false, + load: false, + serverLoad: false, + }); + // @ts-ignore this exists + const loadResult = await plugin.load(path); + expect(loadResult).toEqual(null); + }); + }); +}); + +describe('canWrapLoad', () => { + afterEach(() => { + fileContent = undefined; + }); + + it.each([ + ['export variable declaration - function pointer', 'export const load= loadPageData'], + ['export variable declaration - factory function call', 'export const load =loadPageData()'], + ['export variable declaration - inline function', 'export const load = () => { return { props: { msg: "hi" } } }'], + [ + 'export variable declaration - inline async function', + 'export const load = async () => { return { props: { msg: "hi" } } }', + ], + + ['export function declaration', 'export function load ( ){ return { props: { msg: "hi" } } }'], + [ + 'export function declaration - with params', + `export async function load({fetch}){ + const res = await fetch('https://example.com'); + return { props: { msg: res.toString() } } + }`, + ], + + [ + 'variable declaration (let)', + `import {something} from 'somewhere'; + let load = async () => {}; + export prerender = true; + export { load}`, + ], + [ + 'variable declaration (var)', + `import {something} from 'somewhere'; + var load=async () => {}; + export prerender = true; + export { load}`, + ], + + [ + 'function declaration', + `import {something} from 'somewhere'; + async function load(){}; + export { load }`, + ], + [ + 'function declaration, sentry commented out', + `import {something} from 'somewhere'; + // import * as Sentry from '@sentry/sveltekit'; + async function load(){}; + export { load }`, + ], + [ + 'function declaration, sentry commented out', + `import {something} from 'somewhere'; + /* import * as Sentry from '@sentry/sveltekit'; */ + async function load(){}; + export { load }`, + ], + ])('returns `true` if a load declaration (%s) exists and no Sentry code was found', async (_, code) => { + fileContent = code; + expect(await autoInstument.canWrapLoad('+page.ts', false)).toEqual(true); + }); + + it.each([ + 'export const almostLoad = () => {}; export const prerender = true;', + 'export const loadNotLoad = () => {}; export const prerender = true;', + 'export function aload(){}; export const prerender = true;', + 'export function loader(){}; export const prerender = true;', + 'let loademe = false; export {loadme}', + 'const a = {load: true}; export {a}', + 'if (s === "load") {}', + 'const a = load ? load : false', + '// const load = () => {}', + '/* export const load = () => {} */ export const prerender = true;', + ])('returns `false` if no load declaration exists', async (_, code) => { + fileContent = code; + expect(await autoInstument.canWrapLoad('+page.ts', false)).toEqual(true); + }); + + it('returns `false` if Sentry code was found', async () => { + fileContent = 'import * as Sentry from "@sentry/sveltekit";'; + expect(await autoInstument.canWrapLoad('+page.ts', false)).toEqual(false); + }); + + it('returns `false` if Sentry code was found', async () => { + fileContent = 'import * as Sentry from "@sentry/sveltekit";'; + expect(await autoInstument.canWrapLoad('+page.ts', false)).toEqual(false); + }); +}); diff --git a/packages/sveltekit/test/vite/sentrySvelteKitPlugins.test.ts b/packages/sveltekit/test/vite/sentrySvelteKitPlugins.test.ts index 4479d1d1c0dd..f606b5ee6e9a 100644 --- a/packages/sveltekit/test/vite/sentrySvelteKitPlugins.test.ts +++ b/packages/sveltekit/test/vite/sentrySvelteKitPlugins.test.ts @@ -1,24 +1,46 @@ import { vi } from 'vitest'; +import * as autoInstrument from '../../src/vite/autoInstrument'; import { sentrySvelteKit } from '../../src/vite/sentryVitePlugins'; import * as sourceMaps from '../../src/vite/sourceMaps'; +vi.mock('fs', async () => { + const actual = await vi.importActual('fs'); + return { + // @ts-ignore this exists, I promise! + ...actual, + promises: { + // @ts-ignore this also exists, I promise! + ...actual.promises, + readFile: vi.fn().mockReturnValue('foo'), + }, + }; +}); + describe('sentryVite()', () => { it('returns an array of Vite plugins', async () => { const plugins = await sentrySvelteKit(); + expect(plugins).toBeInstanceOf(Array); - expect(plugins).toHaveLength(1); + expect(plugins).toHaveLength(2); }); - it('returns the custom sentry source maps plugin by default', async () => { + it('returns the custom sentry source maps plugin and the auto-instrument plugin by default', async () => { const plugins = await sentrySvelteKit(); - const plugin = plugins[0]; - expect(plugin.name).toEqual('sentry-vite-plugin-custom'); + const instrumentPlugin = plugins[0]; + const sourceMapsPlugin = plugins[1]; + expect(instrumentPlugin.name).toEqual('sentry-auto-instrumentation'); + expect(sourceMapsPlugin.name).toEqual('sentry-upload-source-maps'); }); it("doesn't return the custom sentry source maps plugin if autoUploadSourcemaps is `false`", async () => { const plugins = await sentrySvelteKit({ autoUploadSourceMaps: false }); - expect(plugins).toHaveLength(0); + expect(plugins).toHaveLength(1); + }); + + it("doesn't return the auto instrument plugin if autoInstrument is `false`", async () => { + const plugins = await sentrySvelteKit({ autoInstrument: false }); + expect(plugins).toHaveLength(1); }); it('passes user-specified vite pugin options to the custom sentry source maps plugin', async () => { @@ -29,13 +51,36 @@ describe('sentryVite()', () => { include: ['foo.js'], ignore: ['bar.js'], }, + autoInstrument: false, }); const plugin = plugins[0]; - expect(plugin.name).toEqual('sentry-vite-plugin-custom'); + + expect(plugin.name).toEqual('sentry-upload-source-maps'); expect(makePluginSpy).toHaveBeenCalledWith({ debug: true, ignore: ['bar.js'], include: ['foo.js'], }); }); + + it('passes user-specified options to the auto instrument plugin', async () => { + const makePluginSpy = vi.spyOn(autoInstrument, 'makeAutoInstrumentationPlugin'); + const plugins = await sentrySvelteKit({ + debug: true, + autoInstrument: { + load: true, + serverLoad: false, + }, + // just to ignore the source maps plugin: + autoUploadSourceMaps: false, + }); + const plugin = plugins[0]; + + expect(plugin.name).toEqual('sentry-auto-instrumentation'); + expect(makePluginSpy).toHaveBeenCalledWith({ + debug: true, + load: true, + serverLoad: false, + }); + }); }); diff --git a/packages/sveltekit/test/vite/sourceMaps.test.ts b/packages/sveltekit/test/vite/sourceMaps.test.ts index 91a1863708b0..7d412811b92e 100644 --- a/packages/sveltekit/test/vite/sourceMaps.test.ts +++ b/packages/sveltekit/test/vite/sourceMaps.test.ts @@ -26,7 +26,7 @@ beforeEach(() => { describe('makeCustomSentryVitePlugin()', () => { it('returns the custom sentry source maps plugin', async () => { const plugin = await makeCustomSentryVitePlugin(); - expect(plugin.name).toEqual('sentry-vite-plugin-custom'); + expect(plugin.name).toEqual('sentry-upload-source-maps'); expect(plugin.apply).toEqual('build'); expect(plugin.enforce).toEqual('post'); From a7fc8331dac941ccf17d56b84aae5a055d7fc105 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 3 May 2023 11:28:12 +0200 Subject: [PATCH 2/4] Apply suggestions from code review --- packages/sveltekit/src/vite/autoInstrument.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/sveltekit/src/vite/autoInstrument.ts b/packages/sveltekit/src/vite/autoInstrument.ts index 4150e873c394..944f19c7837b 100644 --- a/packages/sveltekit/src/vite/autoInstrument.ts +++ b/packages/sveltekit/src/vite/autoInstrument.ts @@ -95,13 +95,13 @@ export async function canWrapLoad(id: string, debug: boolean): Promise const hasSentryContent = codeWithoutComments.includes('@sentry/sveltekit'); if (hasSentryContent) { // eslint-disable-next-line no-console - debug && console.log(`Skipping wrappiung ${id} because it already contains Sentry code`); + debug && console.log(`Skipping wrapping ${id} because it already contains Sentry code`); } const hasLoadDeclaration = /(const|let|var|function)\s+load\s*(=|\()/gm.test(codeWithoutComments); if (!hasLoadDeclaration) { // eslint-disable-next-line no-console - debug && console.log(`Skipping wrappiung ${id} because it doesn't declare a \`load\` function`); + debug && console.log(`Skipping wrapping ${id} because it doesn't declare a \`load\` function`); } return !hasSentryContent && hasLoadDeclaration; From ec3b07698cf029a90525ce4dfd1a55dd7ef60d5e Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 4 May 2023 10:48:15 +0200 Subject: [PATCH 3/4] remove unnecessary async and cleanup tests --- packages/sveltekit/src/vite/autoInstrument.ts | 2 +- .../sveltekit/src/vite/sentryVitePlugins.ts | 2 +- .../test/vite/autoInstrument.test.ts | 21 +++++++++---------- 3 files changed, 12 insertions(+), 13 deletions(-) diff --git a/packages/sveltekit/src/vite/autoInstrument.ts b/packages/sveltekit/src/vite/autoInstrument.ts index 944f19c7837b..2cddf8aad922 100644 --- a/packages/sveltekit/src/vite/autoInstrument.ts +++ b/packages/sveltekit/src/vite/autoInstrument.ts @@ -37,7 +37,7 @@ type AutoInstrumentPluginOptions = AutoInstrumentSelection & { * * @returns the plugin */ -export async function makeAutoInstrumentationPlugin(options: AutoInstrumentPluginOptions): Promise { +export function makeAutoInstrumentationPlugin(options: AutoInstrumentPluginOptions): Plugin { const { load: shouldWrapLoad, serverLoad: shouldWrapServerLoad, debug } = options; return { diff --git a/packages/sveltekit/src/vite/sentryVitePlugins.ts b/packages/sveltekit/src/vite/sentryVitePlugins.ts index 615112744f81..0f2461a419de 100644 --- a/packages/sveltekit/src/vite/sentryVitePlugins.ts +++ b/packages/sveltekit/src/vite/sentryVitePlugins.ts @@ -71,7 +71,7 @@ export async function sentrySvelteKit(options: SentrySvelteKitPluginOptions = {} }; sentryPlugins.push( - await makeAutoInstrumentationPlugin({ + makeAutoInstrumentationPlugin({ ...pluginOptions, debug: options.debug || false, }), diff --git a/packages/sveltekit/test/vite/autoInstrument.test.ts b/packages/sveltekit/test/vite/autoInstrument.test.ts index 421835ca35be..67e7c427cf56 100644 --- a/packages/sveltekit/test/vite/autoInstrument.test.ts +++ b/packages/sveltekit/test/vite/autoInstrument.test.ts @@ -1,7 +1,6 @@ import { vi } from 'vitest'; -// import { makeAutoInstrumentationPlugin } from '../../src/vite/autoInstrument'; -import * as autoInstument from '../../src/vite/autoInstrument'; +import { canWrapLoad, makeAutoInstrumentationPlugin } from '../../src/vite/autoInstrument'; const DEFAULT_CONTENT = ` export const load = () => {}; @@ -38,7 +37,7 @@ describe('makeAutoInstrumentationPlugin()', () => { }); it('returns the auto instrumentation plugin', async () => { - const plugin = await autoInstument.makeAutoInstrumentationPlugin({ debug: true, load: true, serverLoad: true }); + const plugin = makeAutoInstrumentationPlugin({ debug: true, load: true, serverLoad: true }); expect(plugin.name).toEqual('sentry-auto-instrumentation'); expect(plugin.enforce).toEqual('pre'); expect(plugin.load).toEqual(expect.any(Function)); @@ -55,7 +54,7 @@ describe('makeAutoInstrumentationPlugin()', () => { 'path/to/+layout.mjs', ])('transform %s files', (path: string) => { it('wraps universal load if `load` option is `true`', async () => { - const plugin = await autoInstument.makeAutoInstrumentationPlugin({ debug: false, load: true, serverLoad: true }); + const plugin = makeAutoInstrumentationPlugin({ debug: false, load: true, serverLoad: true }); // @ts-ignore this exists const loadResult = await plugin.load(path); expect(loadResult).toEqual( @@ -67,7 +66,7 @@ describe('makeAutoInstrumentationPlugin()', () => { }); it("doesn't wrap universal load if `load` option is `false`", async () => { - const plugin = await autoInstument.makeAutoInstrumentationPlugin({ + const plugin = makeAutoInstrumentationPlugin({ debug: false, load: false, serverLoad: false, @@ -89,7 +88,7 @@ describe('makeAutoInstrumentationPlugin()', () => { 'path/to/+layout.server.mjs', ])('transform %s files', (path: string) => { it('wraps universal load if `load` option is `true`', async () => { - const plugin = await autoInstument.makeAutoInstrumentationPlugin({ debug: false, load: false, serverLoad: true }); + const plugin = makeAutoInstrumentationPlugin({ debug: false, load: false, serverLoad: true }); // @ts-ignore this exists const loadResult = await plugin.load(path); expect(loadResult).toEqual( @@ -101,7 +100,7 @@ describe('makeAutoInstrumentationPlugin()', () => { }); it("doesn't wrap universal load if `load` option is `false`", async () => { - const plugin = await autoInstument.makeAutoInstrumentationPlugin({ + const plugin = makeAutoInstrumentationPlugin({ debug: false, load: false, serverLoad: false, @@ -173,7 +172,7 @@ describe('canWrapLoad', () => { ], ])('returns `true` if a load declaration (%s) exists and no Sentry code was found', async (_, code) => { fileContent = code; - expect(await autoInstument.canWrapLoad('+page.ts', false)).toEqual(true); + expect(await canWrapLoad('+page.ts', false)).toEqual(true); }); it.each([ @@ -189,16 +188,16 @@ describe('canWrapLoad', () => { '/* export const load = () => {} */ export const prerender = true;', ])('returns `false` if no load declaration exists', async (_, code) => { fileContent = code; - expect(await autoInstument.canWrapLoad('+page.ts', false)).toEqual(true); + expect(await canWrapLoad('+page.ts', false)).toEqual(true); }); it('returns `false` if Sentry code was found', async () => { fileContent = 'import * as Sentry from "@sentry/sveltekit";'; - expect(await autoInstument.canWrapLoad('+page.ts', false)).toEqual(false); + expect(await canWrapLoad('+page.ts', false)).toEqual(false); }); it('returns `false` if Sentry code was found', async () => { fileContent = 'import * as Sentry from "@sentry/sveltekit";'; - expect(await autoInstument.canWrapLoad('+page.ts', false)).toEqual(false); + expect(await canWrapLoad('+page.ts', false)).toEqual(false); }); }); From 74617bf674c4b9cd150da3edbfeecfc0863a47ce Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Thu, 4 May 2023 11:43:09 +0200 Subject: [PATCH 4/4] handle `somethingElse as load` not perfect but better than nothing --- packages/sveltekit/src/vite/autoInstrument.ts | 2 +- packages/sveltekit/test/vite/autoInstrument.test.ts | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/packages/sveltekit/src/vite/autoInstrument.ts b/packages/sveltekit/src/vite/autoInstrument.ts index 2cddf8aad922..c30299c6b8a3 100644 --- a/packages/sveltekit/src/vite/autoInstrument.ts +++ b/packages/sveltekit/src/vite/autoInstrument.ts @@ -98,7 +98,7 @@ export async function canWrapLoad(id: string, debug: boolean): Promise debug && console.log(`Skipping wrapping ${id} because it already contains Sentry code`); } - const hasLoadDeclaration = /(const|let|var|function)\s+load\s*(=|\()/gm.test(codeWithoutComments); + 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`); diff --git a/packages/sveltekit/test/vite/autoInstrument.test.ts b/packages/sveltekit/test/vite/autoInstrument.test.ts index 67e7c427cf56..bc1f8d13adb8 100644 --- a/packages/sveltekit/test/vite/autoInstrument.test.ts +++ b/packages/sveltekit/test/vite/autoInstrument.test.ts @@ -170,6 +170,12 @@ describe('canWrapLoad', () => { async function load(){}; export { load }`, ], + [ + 'function declaration with different name', + `import { foo } from 'somewhere'; + async function somethingElse(){}; + export { somethingElse as load, foo }`, + ], ])('returns `true` if a load declaration (%s) exists and no Sentry code was found', async (_, code) => { fileContent = code; expect(await canWrapLoad('+page.ts', false)).toEqual(true); @@ -186,6 +192,7 @@ describe('canWrapLoad', () => { 'const a = load ? load : false', '// const load = () => {}', '/* export const load = () => {} */ export const prerender = true;', + '/* export const notLoad = () => { const a = getSomething() as load; } */ export const prerender = true;', ])('returns `false` if no load declaration exists', async (_, code) => { fileContent = code; expect(await canWrapLoad('+page.ts', false)).toEqual(true);