From 57007c93c1e0fae12663370a573cfa11d209a45a Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Tue, 25 Apr 2023 15:02:36 +0200 Subject: [PATCH 1/2] feat(sveltekit): Auto-wrap universal and server load functions --- packages/sveltekit/.eslintrc.js | 1 + packages/sveltekit/package.json | 6 +- packages/sveltekit/rollup.npm.config.js | 53 ++++-- packages/sveltekit/scripts/buildRollup.ts | 24 +++ packages/sveltekit/src/vite/autoInstrument.ts | 152 ++++++++++++++++++ .../sveltekit/src/vite/sentryVitePlugins.ts | 41 ++++- packages/sveltekit/src/vite/sourceMaps.ts | 8 +- .../src/vite/templates/serverLoadTemplate.ts | 12 ++ .../vite/templates/universalLoadTemplate.ts | 12 ++ .../test/vite/autoInstrument.test.ts | 145 +++++++++++++++++ .../test/vite/sentrySvelteKitPlugins.test.ts | 57 ++++++- .../sveltekit/test/vite/sourceMaps.test.ts | 2 +- packages/sveltekit/tsconfig.json | 2 + 13 files changed, 487 insertions(+), 28 deletions(-) create mode 100644 packages/sveltekit/scripts/buildRollup.ts create mode 100644 packages/sveltekit/src/vite/autoInstrument.ts create mode 100644 packages/sveltekit/src/vite/templates/serverLoadTemplate.ts create mode 100644 packages/sveltekit/src/vite/templates/universalLoadTemplate.ts create mode 100644 packages/sveltekit/test/vite/autoInstrument.test.ts diff --git a/packages/sveltekit/.eslintrc.js b/packages/sveltekit/.eslintrc.js index c3ca0faa363b..71e5469d8057 100644 --- a/packages/sveltekit/.eslintrc.js +++ b/packages/sveltekit/.eslintrc.js @@ -19,4 +19,5 @@ module.exports = { }, ], extends: ['../../.eslintrc.js'], + ignorePatterns: ['scripts/**/*', 'src/vite/templates/**/*'], }; diff --git a/packages/sveltekit/package.json b/packages/sveltekit/package.json index 577cf502d789..bceda7ecc265 100644 --- a/packages/sveltekit/package.json +++ b/packages/sveltekit/package.json @@ -29,11 +29,11 @@ "@sentry/utils": "7.49.0", "@sentry/vite-plugin": "^0.6.0", "magic-string": "^0.30.0", + "rollup": "^3.20.2", "sorcery": "0.11.0" }, "devDependencies": { "@sveltejs/kit": "^1.11.0", - "rollup": "^3.20.2", "svelte": "^3.44.0", "typescript": "^4.9.3", "vite": "4.0.0" @@ -41,11 +41,11 @@ "scripts": { "build": "run-p build:transpile build:types", "build:dev": "yarn build", - "build:transpile": "rollup -c rollup.npm.config.js --bundleConfigAsCjs", + "build:transpile": "ts-node scripts/buildRollup.ts", "build:types": "tsc -p tsconfig.types.json", "build:watch": "run-p build:transpile:watch build:types:watch", "build:dev:watch": "yarn build:watch", - "build:transpile:watch": "rollup -c rollup.npm.config.js --watch", + "build:transpile:watch": "nodemon --ext ts --watch src scripts/buildRollup.ts", "build:types:watch": "tsc -p tsconfig.types.json --watch", "build:tarball": "ts-node ../../scripts/prepack.ts && npm pack ./build", "circularDepCheck": "madge --circular src/index.client.ts && madge --circular src/index.server.ts && madge --circular src/index.types.ts", diff --git a/packages/sveltekit/rollup.npm.config.js b/packages/sveltekit/rollup.npm.config.js index d18b0c102d19..624e177a131d 100644 --- a/packages/sveltekit/rollup.npm.config.js +++ b/packages/sveltekit/rollup.npm.config.js @@ -1,13 +1,44 @@ import { makeBaseNPMConfig, makeNPMConfigVariants } from '../../rollup/index.js'; -export default makeNPMConfigVariants( - makeBaseNPMConfig({ - entrypoints: ['src/index.server.ts', 'src/index.client.ts', 'src/client/index.ts', 'src/server/index.ts'], - packageSpecificConfig: { - external: ['$app/stores'], - output: { - dynamicImportInCjs: true, - } - }, - }), -); +export default [ + + ...makeNPMConfigVariants( + makeBaseNPMConfig({ + entrypoints: ['src/index.server.ts', 'src/index.client.ts', 'src/client/index.ts', 'src/server/index.ts'], + packageSpecificConfig: { + external: ['$app/stores'], + output: { + dynamicImportInCjs: true, + } + }, + }), + ), + // Templates for load function wrappers + ...makeNPMConfigVariants( + makeBaseNPMConfig({ + entrypoints: [ + 'src/vite/templates/universalLoadTemplate.ts', + 'src/vite/templates/serverLoadTemplate.ts', + ], + + packageSpecificConfig: { + output: { + // Preserve the original file structure (i.e., so that everything is still relative to `src`) + entryFileNames: 'vite/templates/[name].js', + + // this is going to be add-on code, so it doesn't need the trappings of a full module (and in fact actively + // shouldn't have them, lest they muck with the module to which we're adding it) + sourcemap: false, + esModule: false, + + // make it so Rollup calms down about the fact that we're combining default and named exports + exports: 'named', + }, + external: [ + '@sentry/sveltekit', + '__SENTRY_WRAPPING_TARGET_FILE__', + ], + }, + }), +), +]; diff --git a/packages/sveltekit/scripts/buildRollup.ts b/packages/sveltekit/scripts/buildRollup.ts new file mode 100644 index 000000000000..acb6a0d3760e --- /dev/null +++ b/packages/sveltekit/scripts/buildRollup.ts @@ -0,0 +1,24 @@ +import * as childProcess from 'child_process'; +import * as fs from 'fs'; +import * as path from 'path'; + +/** + * Run the given shell command, piping the shell process's `stdin`, `stdout`, and `stderr` to that of the current + * process. Returns contents of `stdout`. + */ +function run(cmd: string, options?: childProcess.ExecSyncOptions): string | Buffer { + return childProcess.execSync(cmd, { stdio: 'inherit', ...options }); +} + +run('./node_modules/.bin/rollup -c rollup.npm.config.js --bundleConfigAsCjs'); + +// Regardless of whether SvelteKit is using the CJS or ESM version of our SDK, we want the code from our templates to be in +// ESM (since we'll be adding it onto page files which are themselves written in ESM), so copy the ESM versions of the +// templates over into the CJS build directory. (Building only the ESM version and sticking it in both locations is +// something which in theory Rollup could do, but it would mean refactoring our Rollup helper functions, which isn't +// worth it just for this.) +const cjsTemplateDir = 'build/cjs/vite/templates/'; +const esmTemplateDir = 'build/esm/vite/templates/'; +fs.readdirSync(esmTemplateDir).forEach(templateFile => + fs.copyFileSync(path.join(esmTemplateDir, templateFile), path.join(cjsTemplateDir, templateFile)), +); diff --git a/packages/sveltekit/src/vite/autoInstrument.ts b/packages/sveltekit/src/vite/autoInstrument.ts new file mode 100644 index 000000000000..27e0ccc8e2a1 --- /dev/null +++ b/packages/sveltekit/src/vite/autoInstrument.ts @@ -0,0 +1,152 @@ +/* eslint-disable @sentry-internal/sdk/no-optional-chaining */ +import * as fs from 'fs'; +import * as path from 'path'; +import type { SourceMap } from 'rollup'; +import { rollup } from 'rollup'; +import type { Plugin } from 'vite'; + +// Just a simple placeholder to make referencing module consistent +const SENTRY_WRAPPER_MODULE_NAME = 'sentry-wrapper-module'; + +// Needs to end in .cjs in order for the `commonjs` plugin to pick it up +const WRAPPING_TARGET_MODULE_NAME = '__SENTRY_WRAPPING_TARGET_FILE__.js'; + +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 + * + * @returns the plugin + */ +export async function makeAutoInstrumentationPlugin(options: AutoInstrumentPluginOptions): Promise { + const universalLoadTemplatePath = path.resolve(__dirname, 'templates', 'universalLoadTemplate.js'); + const universalLoadTemplate = (await fs.promises.readFile(universalLoadTemplatePath, 'utf-8')).toString(); + + const serverLoadTemplatePath = path.resolve(__dirname, 'templates', 'serverLoadTemplate.js'); + const serverLoadTemplate = (await fs.promises.readFile(serverLoadTemplatePath, 'utf-8')).toString(); + + const universalLoadWrappingCode = universalLoadTemplate.replace( + /__SENTRY_WRAPPING_TARGET_FILE__/g, + WRAPPING_TARGET_MODULE_NAME, + ); + const serverLoadWrappingCode = serverLoadTemplate.replace( + /__SENTRY_WRAPPING_TARGET_FILE__/g, + WRAPPING_TARGET_MODULE_NAME, + ); + + const { load: shouldWrapLoad, serverLoad: shouldWrapServerLoad, debug } = options; + + return { + name: 'sentry-auto-instrumentation', + enforce: 'post', + async transform(userCode, id) { + const shouldApplyUniversalLoadWrapper = + shouldWrapLoad && + /\+(page|layout)\.(js|ts|mjs|mts)$/.test(id) && + // Simple check to see if users already instrumented the file manually + !userCode.includes('@sentry/sveltekit'); + + if (shouldApplyUniversalLoadWrapper) { + // eslint-disable-next-line no-console + debug && console.log('[Sentry] Applying universal load wrapper to', id); + return await wrapUserCode(universalLoadWrappingCode, userCode); + } + + const shouldApplyServerLoadWrapper = + shouldWrapServerLoad && + /\+(page|layout)\.server\.(js|ts|mjs|mts)$/.test(id) && + !userCode.includes('@sentry/sveltekit'); + + if (shouldApplyServerLoadWrapper) { + // eslint-disable-next-line no-console + debug && console.log('[Sentry] Applying server load wrapper to', id); + return await wrapUserCode(serverLoadWrappingCode, userCode); + } + + return null; + }, + }; +} + +/** + * Uses rollup to bundle the wrapper code and the user code together, so that we can use rollup's source map support. + * This works analogously to our NextJS wrapping solution. + * The one exception is that we don't pass in any source map. This is because generating the userCode's + * source map generally works but it breaks SvelteKit's source map generation for some reason. + * Not passing a map actually works and things are still mapped correctly in the end. + * No Sentry code is visible in the final source map. + * @see {@link file:///./../../../nextjs/src/config/loaders/wrappingLoader.ts} for more details. + */ +async function wrapUserCode( + wrapperCode: string, + userModuleCode: string, +): Promise<{ code: string; map?: SourceMap | null }> { + const rollupBuild = await rollup({ + input: SENTRY_WRAPPER_MODULE_NAME, + + plugins: [ + { + name: 'virtualize-sentry-wrapper-modules', + resolveId: id => { + if (id === SENTRY_WRAPPER_MODULE_NAME || id === WRAPPING_TARGET_MODULE_NAME) { + return id; + } else { + return null; + } + }, + load(id) { + if (id === SENTRY_WRAPPER_MODULE_NAME) { + return wrapperCode; + } else if (id === WRAPPING_TARGET_MODULE_NAME) { + return { + code: userModuleCode, + // map: userModuleSourceMap, + }; + } else { + return null; + } + }, + }, + ], + + external: sourceId => sourceId !== SENTRY_WRAPPER_MODULE_NAME && sourceId !== WRAPPING_TARGET_MODULE_NAME, + + context: 'this', + + makeAbsoluteExternalsRelative: false, + + onwarn: (_warning, _warn) => { + // Suppress all warnings - we don't want to bother people with this output + // _warn(_warning); // uncomment to debug + }, + }); + + const finalBundle = await rollupBuild.generate({ + format: 'esm', + sourcemap: 'hidden', + }); + + return finalBundle.output[0]; +} 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..a279b4fe46e4 100644 --- a/packages/sveltekit/src/vite/sourceMaps.ts +++ b/packages/sveltekit/src/vite/sourceMaps.ts @@ -74,9 +74,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. @@ -85,7 +85,7 @@ export async function makeCustomSentryVitePlugin(options?: SentryVitePluginOptio renderChunk, transform, - // Modify the config to generate source maps + // // Modify the config to generate source maps config: config => { // eslint-disable-next-line no-console debug && console.log('[Source Maps Plugin] Enabeling source map generation'); @@ -117,6 +117,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 diff --git a/packages/sveltekit/src/vite/templates/serverLoadTemplate.ts b/packages/sveltekit/src/vite/templates/serverLoadTemplate.ts new file mode 100644 index 000000000000..c88a61be5e55 --- /dev/null +++ b/packages/sveltekit/src/vite/templates/serverLoadTemplate.ts @@ -0,0 +1,12 @@ +// @ts-ignore - this import placeholed will be replaced! +import * as userModule from '__SENTRY_WRAPPING_TARGET_FILE__'; +// eslint-disable-next-line import/no-extraneous-dependencies +import { wrapServerLoadWithSentry } from '@sentry/sveltekit'; + +// @ts-ignore whatever +// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access +export const load = userModule.load ? wrapServerLoadWithSentry(userModule.load) : undefined; + +// Re-export anything exported by the page module we're wrapping. +// @ts-ignore See import on top +export * from '__SENTRY_WRAPPING_TARGET_FILE__'; diff --git a/packages/sveltekit/src/vite/templates/universalLoadTemplate.ts b/packages/sveltekit/src/vite/templates/universalLoadTemplate.ts new file mode 100644 index 000000000000..23c7fad0bff1 --- /dev/null +++ b/packages/sveltekit/src/vite/templates/universalLoadTemplate.ts @@ -0,0 +1,12 @@ +// @ts-ignore - this import placeholed will be replaced! +import * as userModule from '__SENTRY_WRAPPING_TARGET_FILE__'; +// eslint-disable-next-line import/no-extraneous-dependencies +import { wrapLoadWithSentry } from '@sentry/sveltekit'; + +// @ts-ignore whatever +// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access +export const load = userModule.load ? wrapLoadWithSentry(userModule.load) : undefined; + +// Re-export anything exported by the page module we're wrapping. +// @ts-ignore See import on top +export * from '__SENTRY_WRAPPING_TARGET_FILE__'; diff --git a/packages/sveltekit/test/vite/autoInstrument.test.ts b/packages/sveltekit/test/vite/autoInstrument.test.ts new file mode 100644 index 000000000000..9e0f80377b17 --- /dev/null +++ b/packages/sveltekit/test/vite/autoInstrument.test.ts @@ -0,0 +1,145 @@ +import { vi } from 'vitest'; + +import { makeAutoInstrumentationPlugin } from '../../src/vite/autoInstrument'; + +let returnFileWithSentryContent = false; + +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(() => { + if (returnFileWithSentryContent) { + return "import * as Sentry from '@sentry/sveltekit'"; + } + return 'foo'; + }), + }, + }; +}); + +vi.mock('rollup', () => { + return { + rollup: vi.fn().mockReturnValue({ generate: vi.fn().mockReturnValue({ output: ['transformed'] }) }), + }; +}); + +describe('makeAutoInstrumentationPlugin()', () => { + beforeEach(() => { + vi.clearAllMocks(); + returnFileWithSentryContent = false; + }); + + it('returns the auto instrumentation plugin', async () => { + const plugin = await makeAutoInstrumentationPlugin({ debug: true, load: true, serverLoad: true }); + expect(plugin.name).toEqual('sentry-auto-instrumentation'); + expect(plugin.enforce).toEqual('post'); + expect(plugin.transform).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 makeAutoInstrumentationPlugin({ debug: false, load: true, serverLoad: true }); + // @ts-ignore this exists + const transformResult = await plugin.transform.call( + { + getCombinedSourcemap: vi.fn().mockReturnValue({}), + }, + 'foo', + path, + ); + expect(transformResult).toEqual('transformed'); + }); + + it("doesn't wrap universal load if the file already contains Sentry code", async () => { + returnFileWithSentryContent = true; + const plugin = await makeAutoInstrumentationPlugin({ debug: false, load: true, serverLoad: false }); + // @ts-ignore this exists + const transformResult = await plugin.transform.call( + { + getCombinedSourcemap: vi.fn().mockReturnValue({}), + }, + 'foo', + path, + ); + expect(transformResult).toEqual('transformed'); + }); + + it("doesn't wrap universal load if `load` option is `false`", async () => { + const plugin = await makeAutoInstrumentationPlugin({ debug: false, load: false, serverLoad: false }); + // @ts-ignore this exists + const transformResult = await plugin.transform.call( + { + getCombinedSourcemap: vi.fn().mockReturnValue({}), + }, + 'foo', + path, + ); + expect(transformResult).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 makeAutoInstrumentationPlugin({ debug: false, load: false, serverLoad: true }); + // @ts-ignore this exists + const transformResult = await plugin.transform.call( + { + getCombinedSourcemap: vi.fn().mockReturnValue({}), + }, + 'foo', + path, + ); + expect(transformResult).toEqual('transformed'); + }); + + it("doesn't wrap universal load if the file already contains Sentry code", async () => { + returnFileWithSentryContent = true; + const plugin = await makeAutoInstrumentationPlugin({ debug: false, load: false, serverLoad: true }); + // @ts-ignore this exists + const transformResult = await plugin.transform.call( + { + getCombinedSourcemap: vi.fn().mockReturnValue({}), + }, + 'foo', + path, + ); + expect(transformResult).toEqual('transformed'); + }); + + it("doesn't wrap universal load if `load` option is `false`", async () => { + const plugin = await makeAutoInstrumentationPlugin({ debug: false, load: false, serverLoad: false }); + // @ts-ignore this exists + const transformResult = await plugin.transform.call( + { + getCombinedSourcemap: vi.fn().mockReturnValue({}), + }, + 'foo', + path, + ); + expect(transformResult).toEqual(null); + }); + }); +}); 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'); diff --git a/packages/sveltekit/tsconfig.json b/packages/sveltekit/tsconfig.json index bf45a09f2d71..f4acf6b67ab9 100644 --- a/packages/sveltekit/tsconfig.json +++ b/packages/sveltekit/tsconfig.json @@ -3,6 +3,8 @@ "include": ["src/**/*"], + "exclude": ["src/vite/templates/**/*"], + "compilerOptions": { // package-specific options } From a74d6e674b3afc1266df33ecf5890e86ae698149 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 26 Apr 2023 13:09:18 +0200 Subject: [PATCH 2/2] switch to AST-based wrapping --- packages/sveltekit/.eslintrc.js | 1 - packages/sveltekit/package.json | 8 +- packages/sveltekit/rollup.npm.config.js | 53 +---- packages/sveltekit/scripts/buildRollup.ts | 24 --- packages/sveltekit/src/vite/autoInstrument.ts | 194 +++++++++++------- .../src/vite/templates/serverLoadTemplate.ts | 12 -- .../vite/templates/universalLoadTemplate.ts | 12 -- .../test/vite/autoInstrument.test.ts | 194 ++++++++++++++---- yarn.lock | 41 ++++ 9 files changed, 333 insertions(+), 206 deletions(-) delete mode 100644 packages/sveltekit/scripts/buildRollup.ts delete mode 100644 packages/sveltekit/src/vite/templates/serverLoadTemplate.ts delete mode 100644 packages/sveltekit/src/vite/templates/universalLoadTemplate.ts diff --git a/packages/sveltekit/.eslintrc.js b/packages/sveltekit/.eslintrc.js index 71e5469d8057..c3ca0faa363b 100644 --- a/packages/sveltekit/.eslintrc.js +++ b/packages/sveltekit/.eslintrc.js @@ -19,5 +19,4 @@ module.exports = { }, ], extends: ['../../.eslintrc.js'], - ignorePatterns: ['scripts/**/*', 'src/vite/templates/**/*'], }; diff --git a/packages/sveltekit/package.json b/packages/sveltekit/package.json index bceda7ecc265..207e6c0d1322 100644 --- a/packages/sveltekit/package.json +++ b/packages/sveltekit/package.json @@ -28,12 +28,12 @@ "@sentry/types": "7.49.0", "@sentry/utils": "7.49.0", "@sentry/vite-plugin": "^0.6.0", - "magic-string": "^0.30.0", - "rollup": "^3.20.2", + "magicast": "0.2.4", "sorcery": "0.11.0" }, "devDependencies": { "@sveltejs/kit": "^1.11.0", + "rollup": "^3.20.2", "svelte": "^3.44.0", "typescript": "^4.9.3", "vite": "4.0.0" @@ -41,11 +41,11 @@ "scripts": { "build": "run-p build:transpile build:types", "build:dev": "yarn build", - "build:transpile": "ts-node scripts/buildRollup.ts", + "build:transpile": "rollup -c rollup.npm.config.js --bundleConfigAsCjs", "build:types": "tsc -p tsconfig.types.json", "build:watch": "run-p build:transpile:watch build:types:watch", "build:dev:watch": "yarn build:watch", - "build:transpile:watch": "nodemon --ext ts --watch src scripts/buildRollup.ts", + "build:transpile:watch": "rollup -c rollup.npm.config.js --watch", "build:types:watch": "tsc -p tsconfig.types.json --watch", "build:tarball": "ts-node ../../scripts/prepack.ts && npm pack ./build", "circularDepCheck": "madge --circular src/index.client.ts && madge --circular src/index.server.ts && madge --circular src/index.types.ts", diff --git a/packages/sveltekit/rollup.npm.config.js b/packages/sveltekit/rollup.npm.config.js index 624e177a131d..d18b0c102d19 100644 --- a/packages/sveltekit/rollup.npm.config.js +++ b/packages/sveltekit/rollup.npm.config.js @@ -1,44 +1,13 @@ import { makeBaseNPMConfig, makeNPMConfigVariants } from '../../rollup/index.js'; -export default [ - - ...makeNPMConfigVariants( - makeBaseNPMConfig({ - entrypoints: ['src/index.server.ts', 'src/index.client.ts', 'src/client/index.ts', 'src/server/index.ts'], - packageSpecificConfig: { - external: ['$app/stores'], - output: { - dynamicImportInCjs: true, - } - }, - }), - ), - // Templates for load function wrappers - ...makeNPMConfigVariants( - makeBaseNPMConfig({ - entrypoints: [ - 'src/vite/templates/universalLoadTemplate.ts', - 'src/vite/templates/serverLoadTemplate.ts', - ], - - packageSpecificConfig: { - output: { - // Preserve the original file structure (i.e., so that everything is still relative to `src`) - entryFileNames: 'vite/templates/[name].js', - - // this is going to be add-on code, so it doesn't need the trappings of a full module (and in fact actively - // shouldn't have them, lest they muck with the module to which we're adding it) - sourcemap: false, - esModule: false, - - // make it so Rollup calms down about the fact that we're combining default and named exports - exports: 'named', - }, - external: [ - '@sentry/sveltekit', - '__SENTRY_WRAPPING_TARGET_FILE__', - ], - }, - }), -), -]; +export default makeNPMConfigVariants( + makeBaseNPMConfig({ + entrypoints: ['src/index.server.ts', 'src/index.client.ts', 'src/client/index.ts', 'src/server/index.ts'], + packageSpecificConfig: { + external: ['$app/stores'], + output: { + dynamicImportInCjs: true, + } + }, + }), +); diff --git a/packages/sveltekit/scripts/buildRollup.ts b/packages/sveltekit/scripts/buildRollup.ts deleted file mode 100644 index acb6a0d3760e..000000000000 --- a/packages/sveltekit/scripts/buildRollup.ts +++ /dev/null @@ -1,24 +0,0 @@ -import * as childProcess from 'child_process'; -import * as fs from 'fs'; -import * as path from 'path'; - -/** - * Run the given shell command, piping the shell process's `stdin`, `stdout`, and `stderr` to that of the current - * process. Returns contents of `stdout`. - */ -function run(cmd: string, options?: childProcess.ExecSyncOptions): string | Buffer { - return childProcess.execSync(cmd, { stdio: 'inherit', ...options }); -} - -run('./node_modules/.bin/rollup -c rollup.npm.config.js --bundleConfigAsCjs'); - -// Regardless of whether SvelteKit is using the CJS or ESM version of our SDK, we want the code from our templates to be in -// ESM (since we'll be adding it onto page files which are themselves written in ESM), so copy the ESM versions of the -// templates over into the CJS build directory. (Building only the ESM version and sticking it in both locations is -// something which in theory Rollup could do, but it would mean refactoring our Rollup helper functions, which isn't -// worth it just for this.) -const cjsTemplateDir = 'build/cjs/vite/templates/'; -const esmTemplateDir = 'build/esm/vite/templates/'; -fs.readdirSync(esmTemplateDir).forEach(templateFile => - fs.copyFileSync(path.join(esmTemplateDir, templateFile), path.join(cjsTemplateDir, templateFile)), -); diff --git a/packages/sveltekit/src/vite/autoInstrument.ts b/packages/sveltekit/src/vite/autoInstrument.ts index 27e0ccc8e2a1..13c6778a3440 100644 --- a/packages/sveltekit/src/vite/autoInstrument.ts +++ b/packages/sveltekit/src/vite/autoInstrument.ts @@ -1,16 +1,15 @@ /* eslint-disable @sentry-internal/sdk/no-optional-chaining */ -import * as fs from 'fs'; -import * as path from 'path'; -import type { SourceMap } from 'rollup'; -import { rollup } from 'rollup'; +import type { + ExportNamedDeclaration, + FunctionDeclaration, + Program, + VariableDeclaration, + VariableDeclarator, +} from '@babel/types'; +import type { ProxifiedModule } from 'magicast'; +import { builders, generateCode, parseModule } from 'magicast'; import type { Plugin } from 'vite'; -// Just a simple placeholder to make referencing module consistent -const SENTRY_WRAPPER_MODULE_NAME = 'sentry-wrapper-module'; - -// Needs to end in .cjs in order for the `commonjs` plugin to pick it up -const WRAPPING_TARGET_MODULE_NAME = '__SENTRY_WRAPPING_TARGET_FILE__.js'; - export type AutoInstrumentSelection = { /** * If this flag is `true`, the Sentry plugins will automatically instrument the `load` function of @@ -41,21 +40,6 @@ type AutoInstrumentPluginOptions = AutoInstrumentSelection & { * @returns the plugin */ export async function makeAutoInstrumentationPlugin(options: AutoInstrumentPluginOptions): Promise { - const universalLoadTemplatePath = path.resolve(__dirname, 'templates', 'universalLoadTemplate.js'); - const universalLoadTemplate = (await fs.promises.readFile(universalLoadTemplatePath, 'utf-8')).toString(); - - const serverLoadTemplatePath = path.resolve(__dirname, 'templates', 'serverLoadTemplate.js'); - const serverLoadTemplate = (await fs.promises.readFile(serverLoadTemplatePath, 'utf-8')).toString(); - - const universalLoadWrappingCode = universalLoadTemplate.replace( - /__SENTRY_WRAPPING_TARGET_FILE__/g, - WRAPPING_TARGET_MODULE_NAME, - ); - const serverLoadWrappingCode = serverLoadTemplate.replace( - /__SENTRY_WRAPPING_TARGET_FILE__/g, - WRAPPING_TARGET_MODULE_NAME, - ); - const { load: shouldWrapLoad, serverLoad: shouldWrapServerLoad, debug } = options; return { @@ -71,7 +55,8 @@ export async function makeAutoInstrumentationPlugin(options: AutoInstrumentPlugi if (shouldApplyUniversalLoadWrapper) { // eslint-disable-next-line no-console debug && console.log('[Sentry] Applying universal load wrapper to', id); - return await wrapUserCode(universalLoadWrappingCode, userCode); + const wrappedCode = wrapLoad(userCode, 'wrapLoadWithSentry'); + return { code: wrappedCode, map: null }; } const shouldApplyServerLoadWrapper = @@ -82,7 +67,8 @@ export async function makeAutoInstrumentationPlugin(options: AutoInstrumentPlugi if (shouldApplyServerLoadWrapper) { // eslint-disable-next-line no-console debug && console.log('[Sentry] Applying server load wrapper to', id); - return await wrapUserCode(serverLoadWrappingCode, userCode); + const wrappedCode = wrapLoad(userCode, 'wrapServerLoadWithSentry'); + return { code: wrappedCode, map: null }; } return null; @@ -91,62 +77,118 @@ export async function makeAutoInstrumentationPlugin(options: AutoInstrumentPlugi } /** - * Uses rollup to bundle the wrapper code and the user code together, so that we can use rollup's source map support. - * This works analogously to our NextJS wrapping solution. - * The one exception is that we don't pass in any source map. This is because generating the userCode's - * source map generally works but it breaks SvelteKit's source map generation for some reason. - * Not passing a map actually works and things are still mapped correctly in the end. - * No Sentry code is visible in the final source map. - * @see {@link file:///./../../../nextjs/src/config/loaders/wrappingLoader.ts} for more details. + * Applies the wrapLoadWithSentry wrapper to the user's load functions */ -async function wrapUserCode( - wrapperCode: string, - userModuleCode: string, -): Promise<{ code: string; map?: SourceMap | null }> { - const rollupBuild = await rollup({ - input: SENTRY_WRAPPER_MODULE_NAME, - - plugins: [ - { - name: 'virtualize-sentry-wrapper-modules', - resolveId: id => { - if (id === SENTRY_WRAPPER_MODULE_NAME || id === WRAPPING_TARGET_MODULE_NAME) { - return id; - } else { - return null; - } - }, - load(id) { - if (id === SENTRY_WRAPPER_MODULE_NAME) { - return wrapperCode; - } else if (id === WRAPPING_TARGET_MODULE_NAME) { - return { - code: userModuleCode, - // map: userModuleSourceMap, - }; - } else { - return null; - } - }, - }, - ], +// eslint-disable-next-line @typescript-eslint/no-explicit-any +function wrapLoad( + userCode: Readonly, + wrapperFunction: 'wrapLoadWithSentry' | 'wrapServerLoadWithSentry', +): string { + const mod = parseModule(userCode); + + const modAST = mod.exports.$ast as Program; + const namedExports = modAST.body.filter( + (node): node is ExportNamedDeclaration => node.type === 'ExportNamedDeclaration', + ); - external: sourceId => sourceId !== SENTRY_WRAPPER_MODULE_NAME && sourceId !== WRAPPING_TARGET_MODULE_NAME, + let wrappedSucessfully = false; + namedExports.forEach(modExport => { + const declaration = modExport.declaration; + if (!declaration) { + return; + } + if (declaration.type === 'FunctionDeclaration') { + if (!declaration.id || declaration.id.name !== 'load') { + return; + } + const declarationCode = generateCode(declaration).code; + mod.exports.load = builders.raw(`${wrapperFunction}(${declarationCode.replace('load', '_load')})`); + // because of an issue with magicast, we need to remove the original export + modAST.body = modAST.body.filter(node => node !== modExport); + wrappedSucessfully = true; + } else if (declaration.type === 'VariableDeclaration') { + declaration.declarations.forEach(declarator => { + wrappedSucessfully = wrapDeclarator(declarator, wrapperFunction); + }); + } + }); - context: 'this', + if (wrappedSucessfully) { + return generateFinalCode(mod, wrapperFunction); + } - makeAbsoluteExternalsRelative: false, + // If we're here, we know that we didn't find a directly exported `load` function yet. + // We need to look for it in the top level declarations in case it's declared and exported separately. + // First case: top level variable declaration + const topLevelVariableDeclarations = modAST.body.filter( + (statement): statement is VariableDeclaration => statement.type === 'VariableDeclaration', + ); - onwarn: (_warning, _warn) => { - // Suppress all warnings - we don't want to bother people with this output - // _warn(_warning); // uncomment to debug - }, + topLevelVariableDeclarations.forEach(declaration => { + declaration.declarations.forEach(declarator => { + wrappedSucessfully = wrapDeclarator(declarator, wrapperFunction); + }); }); - const finalBundle = await rollupBuild.generate({ - format: 'esm', - sourcemap: 'hidden', + if (wrappedSucessfully) { + return generateFinalCode(mod, wrapperFunction); + } + + // Second case: top level function declaration + // This is the most intrusive modification, as we need to replace a top level function declaration with a + // variable declaration and a function assignment. This changes the spacing formatting of the declarations + // but the line numbers should stay the same + const topLevelFunctionDeclarations = modAST.body.filter( + (statement): statement is FunctionDeclaration => statement.type === 'FunctionDeclaration', + ); + + topLevelFunctionDeclarations.forEach(declaration => { + if (!declaration.id || declaration.id.name !== 'load') { + return; + } + + const stmtIndex = modAST.body.indexOf(declaration); + const declarationCode = generateCode(declaration).code; + const wrappedFunctionBody = builders.raw(`${wrapperFunction}(${declarationCode.replace('load', '_load')})`); + const stringifiedFunctionBody = generateCode(wrappedFunctionBody, {}).code; + + const tmpMod = parseModule(`const load = ${stringifiedFunctionBody}`); + const newDeclarationNode = (tmpMod.$ast as Program).body[0]; + const nodeWithAdjustedLoc = { + ...newDeclarationNode, + loc: { + ...declaration.loc, + }, + }; + + // @ts-ignore - this works, magicast can handle this assignement although the types disagree + modAST.body[stmtIndex] = nodeWithAdjustedLoc; + wrappedSucessfully = true; }); - return finalBundle.output[0]; + if (wrappedSucessfully) { + return generateFinalCode(mod, wrapperFunction); + } + + // nothing found, so we just return the original code + return userCode; +} + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +function generateFinalCode(mod: ProxifiedModule, wrapperFunction: string): string { + const { code } = generateCode(mod); + return `import { ${wrapperFunction} } from '@sentry/sveltekit'; ${code}`; +} + +function wrapDeclarator(declarator: VariableDeclarator, wrapperFunction: string): boolean { + // @ts-ignore - id should always have a name in this case + if (!declarator.id || declarator.id.name !== 'load') { + return false; + } + const declarationInitCode = declarator.init; + // @ts-ignore - we can just place a string here, magicast will convert it to a node + const stringifiedCode = generateCode(declarationInitCode).code; + // @ts-ignore - we can just place a string here, magicast will convert it to a node + declarator.init = `${wrapperFunction}(${stringifiedCode})`; + return true; } diff --git a/packages/sveltekit/src/vite/templates/serverLoadTemplate.ts b/packages/sveltekit/src/vite/templates/serverLoadTemplate.ts deleted file mode 100644 index c88a61be5e55..000000000000 --- a/packages/sveltekit/src/vite/templates/serverLoadTemplate.ts +++ /dev/null @@ -1,12 +0,0 @@ -// @ts-ignore - this import placeholed will be replaced! -import * as userModule from '__SENTRY_WRAPPING_TARGET_FILE__'; -// eslint-disable-next-line import/no-extraneous-dependencies -import { wrapServerLoadWithSentry } from '@sentry/sveltekit'; - -// @ts-ignore whatever -// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access -export const load = userModule.load ? wrapServerLoadWithSentry(userModule.load) : undefined; - -// Re-export anything exported by the page module we're wrapping. -// @ts-ignore See import on top -export * from '__SENTRY_WRAPPING_TARGET_FILE__'; diff --git a/packages/sveltekit/src/vite/templates/universalLoadTemplate.ts b/packages/sveltekit/src/vite/templates/universalLoadTemplate.ts deleted file mode 100644 index 23c7fad0bff1..000000000000 --- a/packages/sveltekit/src/vite/templates/universalLoadTemplate.ts +++ /dev/null @@ -1,12 +0,0 @@ -// @ts-ignore - this import placeholed will be replaced! -import * as userModule from '__SENTRY_WRAPPING_TARGET_FILE__'; -// eslint-disable-next-line import/no-extraneous-dependencies -import { wrapLoadWithSentry } from '@sentry/sveltekit'; - -// @ts-ignore whatever -// eslint-disable-next-line @typescript-eslint/no-unsafe-member-access -export const load = userModule.load ? wrapLoadWithSentry(userModule.load) : undefined; - -// Re-export anything exported by the page module we're wrapping. -// @ts-ignore See import on top -export * from '__SENTRY_WRAPPING_TARGET_FILE__'; diff --git a/packages/sveltekit/test/vite/autoInstrument.test.ts b/packages/sveltekit/test/vite/autoInstrument.test.ts index 9e0f80377b17..6b4244d857bd 100644 --- a/packages/sveltekit/test/vite/autoInstrument.test.ts +++ b/packages/sveltekit/test/vite/autoInstrument.test.ts @@ -2,36 +2,17 @@ import { vi } from 'vitest'; import { makeAutoInstrumentationPlugin } from '../../src/vite/autoInstrument'; -let returnFileWithSentryContent = false; +const BASIC_LOAD_FUNCTION_CALL = 'export const load = () => {}'; -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(() => { - if (returnFileWithSentryContent) { - return "import * as Sentry from '@sentry/sveltekit'"; - } - return 'foo'; - }), - }, - }; -}); - -vi.mock('rollup', () => { - return { - rollup: vi.fn().mockReturnValue({ generate: vi.fn().mockReturnValue({ output: ['transformed'] }) }), - }; -}); +function getWrappedBasicLoadFunctionCall(server = false) { + return `import { wrap${server ? 'Server' : ''}LoadWithSentry } from '@sentry/sveltekit'; export const load = wrap${ + server ? 'Server' : '' + }LoadWithSentry(() => {})`; +} describe('makeAutoInstrumentationPlugin()', () => { beforeEach(() => { vi.clearAllMocks(); - returnFileWithSentryContent = false; }); it('returns the auto instrumentation plugin', async () => { @@ -58,24 +39,26 @@ describe('makeAutoInstrumentationPlugin()', () => { { getCombinedSourcemap: vi.fn().mockReturnValue({}), }, - 'foo', + BASIC_LOAD_FUNCTION_CALL, path, ); - expect(transformResult).toEqual('transformed'); + expect(transformResult).toEqual({ + code: getWrappedBasicLoadFunctionCall(), + map: null, + }); }); it("doesn't wrap universal load if the file already contains Sentry code", async () => { - returnFileWithSentryContent = true; const plugin = await makeAutoInstrumentationPlugin({ debug: false, load: true, serverLoad: false }); // @ts-ignore this exists const transformResult = await plugin.transform.call( { getCombinedSourcemap: vi.fn().mockReturnValue({}), }, - 'foo', + `import { something } from "@sentry/sveltekit";${BASIC_LOAD_FUNCTION_CALL}`, path, ); - expect(transformResult).toEqual('transformed'); + expect(transformResult).toEqual(null); }); it("doesn't wrap universal load if `load` option is `false`", async () => { @@ -109,24 +92,26 @@ describe('makeAutoInstrumentationPlugin()', () => { { getCombinedSourcemap: vi.fn().mockReturnValue({}), }, - 'foo', + BASIC_LOAD_FUNCTION_CALL, path, ); - expect(transformResult).toEqual('transformed'); + expect(transformResult).toEqual({ + code: getWrappedBasicLoadFunctionCall(true), + map: null, + }); }); it("doesn't wrap universal load if the file already contains Sentry code", async () => { - returnFileWithSentryContent = true; const plugin = await makeAutoInstrumentationPlugin({ debug: false, load: false, serverLoad: true }); // @ts-ignore this exists const transformResult = await plugin.transform.call( { getCombinedSourcemap: vi.fn().mockReturnValue({}), }, - 'foo', + `import { wrapServerLoadWithSentry } from "@sentry/sveltekit";${BASIC_LOAD_FUNCTION_CALL}`, path, ); - expect(transformResult).toEqual('transformed'); + expect(transformResult).toEqual(null); }); it("doesn't wrap universal load if `load` option is `false`", async () => { @@ -142,4 +127,143 @@ describe('makeAutoInstrumentationPlugin()', () => { expect(transformResult).toEqual(null); }); }); + + it.each([ + [ + 'export variable declaration - function pointer', + 'export const load = loadPageData', + "import { wrapLoadWithSentry } from '@sentry/sveltekit'; export const load = wrapLoadWithSentry(loadPageData)", + ], + [ + 'export variable declaration - factory function call', + 'export const load = loadPageData()', + "import { wrapLoadWithSentry } from '@sentry/sveltekit'; export const load = wrapLoadWithSentry(loadPageData())", + ], + [ + 'export variable declaration - inline function', + 'export const load = () => { return { props: { msg: "hi" } } }', + 'import { wrapLoadWithSentry } from \'@sentry/sveltekit\'; export const load = wrapLoadWithSentry(() => { return { props: { msg: "hi" } } })', + ], + [ + 'export variable declaration - inline async function', + 'export const load = async () => { return { props: { msg: "hi" } } }', + 'import { wrapLoadWithSentry } from \'@sentry/sveltekit\'; export const load = wrapLoadWithSentry(async () => { return { props: { msg: "hi" } } })', + ], + [ + 'export variable declaration - inline multiline function', + `export const load = async ({fetch}) => { + const res = await fetch('https://example.com'); + return { + props: { + msg: res.toString(), + } + } + }`, + `import { wrapLoadWithSentry } from '@sentry/sveltekit'; export const load = wrapLoadWithSentry(async ({fetch}) => { + const res = await fetch('https://example.com'); + return { + props: { + msg: res.toString(), + } + } + })`, + ], + [ + 'export variable declaration - undefined', + 'export const load = undefined', + "import { wrapLoadWithSentry } from '@sentry/sveltekit'; export const load = wrapLoadWithSentry(undefined)", + ], + [ + 'export variable declaration - null', + 'export const load = null', + "import { wrapLoadWithSentry } from '@sentry/sveltekit'; export const load = wrapLoadWithSentry(null)", + ], + [ + 'export function declaration - simple', + 'export function load () { return { props: { msg: "hi" } } }', + 'import { wrapLoadWithSentry } from \'@sentry/sveltekit\'; export const load = wrapLoadWithSentry(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() } } + }`, + `import { wrapLoadWithSentry } from '@sentry/sveltekit'; export const load = wrapLoadWithSentry(async function _load({fetch}) { + const res = await fetch('https://example.com'); + return { props: { msg: res.toString() } } + });`, + ], + [ + 'variable declaration', + `import {something} from 'somewhere'; + const load = async ({fetch}) => { + const res = await fetch('https://example.com'); + return { props: { msg: res.toString() } } + } + export { load}`, + `import { wrapLoadWithSentry } from '@sentry/sveltekit'; import {something} from 'somewhere'; + const load = wrapLoadWithSentry(async ({fetch}) => { + const res = await fetch('https://example.com'); + return { props: { msg: res.toString() } } + }) + export { load}`, + ], + [ + 'function declaration', + `import {something} from 'somewhere'; + async function load({fetch}) { + const res = await fetch('https://example.com'); + return { props: { msg: res.toString() } }; + } + export { load }`, + `import { wrapLoadWithSentry } from '@sentry/sveltekit'; import {something} from 'somewhere'; +const load = wrapLoadWithSentry(async function _load({fetch}) { + const res = await fetch('https://example.com'); + return { props: { msg: res.toString() } }; +}); +export { load }`, + ], + [ + 'function declaration + other exports', + `import {something} from 'somewhere'; + const prerender = false; + async function load({fetch}) { + const res = await fetch('https://example.com'); + return { props: { msg: res.toString() } } + } + export { load, prerender }`, + `import { wrapLoadWithSentry } from '@sentry/sveltekit'; import {something} from 'somewhere'; +const prerender = false; +const load = wrapLoadWithSentry(async function _load({fetch}) { + const res = await fetch('https://example.com'); + return { props: { msg: res.toString() } } +}); +export { load, prerender }`, + ], + [ + 'file without load', + 'export const prerender = true\nexport const csr = true', + 'export const prerender = true\nexport const csr = true', + ], + ['commented out load', '// export const load = () => {}', '// export const load = () => {}'], + ['commented out load', '// const load = () => {}', '// const load = () => {}'], + ])( + 'correctly modifies the AST to wrap the load function (%s)', + async (_: string, originalCode: string, wrappedCode: string) => { + const plugin = await makeAutoInstrumentationPlugin({ debug: false, load: true, serverLoad: false }); + // @ts-ignore this exists + const transformResult = await plugin.transform.call( + { + getCombinedSourcemap: vi.fn().mockReturnValue({}), + }, + originalCode, + 'path/to/+page.ts', + ); + expect(transformResult).toEqual({ + code: wrappedCode, + map: null, + }); + }, + ); }); diff --git a/yarn.lock b/yarn.lock index a20a58bc37d9..d72272f388f0 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1027,6 +1027,11 @@ resolved "https://registry.yarnpkg.com/@babel/parser/-/parser-7.20.15.tgz#eec9f36d8eaf0948bb88c87a46784b5ee9fd0c89" integrity sha512-DI4a1oZuf8wC+oAJA9RW6ga3Zbe8RZFt7kD9i4qAspz3I/yHet1VvC3DiSy/fsUvv5pvJuNPh0LPOdCcqinDPg== +"@babel/parser@^7.21.4": + version "7.21.4" + resolved "https://registry.yarnpkg.com/@babel/parser/-/parser-7.21.4.tgz#94003fdfc520bbe2875d4ae557b43ddb6d880f17" + integrity sha512-alVJj7k7zIxqBZ7BTRhz0IqJFxW1VJbm6N8JbcYhQ186df9ZBPbZBmWSqAMXwHGsCJdYks7z/voa3ibiS5bCIw== + "@babel/plugin-bugfix-safari-id-destructuring-collision-in-function-expression@^7.18.6": version "7.18.6" resolved "https://registry.yarnpkg.com/@babel/plugin-bugfix-safari-id-destructuring-collision-in-function-expression/-/plugin-bugfix-safari-id-destructuring-collision-in-function-expression-7.18.6.tgz#da5b8f9a580acdfbe53494dba45ea389fb09a4d2" @@ -2218,6 +2223,15 @@ "@babel/helper-validator-identifier" "^7.19.1" to-fast-properties "^2.0.0" +"@babel/types@^7.21.4": + version "7.21.4" + resolved "https://registry.yarnpkg.com/@babel/types/-/types-7.21.4.tgz#2d5d6bb7908699b3b416409ffd3b5daa25b030d4" + integrity sha512-rU2oY501qDxE8Pyo7i/Orqma4ziCOrby0/9mvbDUGEfvZjb279Nk9k19e2fiCxHbRRpY2ZyrgW1eq22mvmOIzA== + dependencies: + "@babel/helper-string-parser" "^7.19.4" + "@babel/helper-validator-identifier" "^7.19.1" + to-fast-properties "^2.0.0" + "@bcoe/v8-coverage@^0.2.3": version "0.2.3" resolved "https://registry.yarnpkg.com/@bcoe/v8-coverage/-/v8-coverage-0.2.3.tgz#75a2e8b51cb758a7553d6804a5932d7aace75c39" @@ -6565,6 +6579,13 @@ ast-types@0.14.2: dependencies: tslib "^2.0.1" +ast-types@0.15.2: + version "0.15.2" + resolved "https://registry.yarnpkg.com/ast-types/-/ast-types-0.15.2.tgz#39ae4809393c4b16df751ee563411423e85fb49d" + integrity sha512-c27loCv9QkZinsa5ProX751khO9DJl/AcB5c2KNtA6NRvHKS0PgLfcftz72KVq504vB0Gku5s2kUZzDBvQWvHg== + dependencies: + tslib "^2.0.1" + astral-regex@^2.0.0: version "2.0.0" resolved "https://registry.yarnpkg.com/astral-regex/-/astral-regex-2.0.0.tgz#483143c567aeed4785759c0865786dc77d7d2e31" @@ -17979,6 +18000,15 @@ magic-string@^0.30.0: dependencies: "@jridgewell/sourcemap-codec" "^1.4.13" +magicast@0.2.4: + version "0.2.4" + resolved "https://registry.yarnpkg.com/magicast/-/magicast-0.2.4.tgz#73cf3e88166ec9db724709b1871553b3e6817400" + integrity sha512-MQPwvpNLQTqaBz0WUqupzDF/Tj9mGXbR2vgENloovPmikiHXzk56HICC+d1LdLqzcLLohEHDd01B43byzr+3tg== + dependencies: + "@babel/parser" "^7.21.4" + "@babel/types" "^7.21.4" + recast "^0.22.0" + make-dir@3.1.0, make-dir@^3.0.0, make-dir@^3.0.2, make-dir@^3.1.0, make-dir@~3.1.0: version "3.1.0" resolved "https://registry.yarnpkg.com/make-dir/-/make-dir-3.1.0.tgz#415e967046b3a7f1d185277d84aa58203726a13f" @@ -22909,6 +22939,17 @@ recast@^0.20.5: source-map "~0.6.1" tslib "^2.0.1" +recast@^0.22.0: + version "0.22.0" + resolved "https://registry.yarnpkg.com/recast/-/recast-0.22.0.tgz#1dd3bf1b86e5eb810b044221a1a734234ed3e9c0" + integrity sha512-5AAx+mujtXijsEavc5lWXBPQqrM4+Dl5qNH96N2aNeuJFUzpiiToKPsxQD/zAIJHspz7zz0maX0PCtCTFVlixQ== + dependencies: + assert "^2.0.0" + ast-types "0.15.2" + esprima "~4.0.0" + source-map "~0.6.1" + tslib "^2.0.1" + rechoir@^0.6.2: version "0.6.2" resolved "https://registry.yarnpkg.com/rechoir/-/rechoir-0.6.2.tgz#85204b54dba82d5742e28c96756ef43af50e3384"