Skip to content

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

Merged
merged 4 commits into from
May 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion packages/sveltekit/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
8 changes: 8 additions & 0 deletions packages/sveltekit/src/server/utils.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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;
}
123 changes: 123 additions & 0 deletions packages/sveltekit/src/vite/autoInstrument.ts
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> {
Copy link
Contributor

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 👍

Copy link
Member Author

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 😅

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
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

41 changes: 37 additions & 4 deletions packages/sveltekit/src/vite/sentryVitePlugins.ts
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;

Expand All @@ -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,
Copy link
Contributor

Choose a reason for hiding this comment

The 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).

Copy link
Member Author

Choose a reason for hiding this comment

The 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,
};

Expand All @@ -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 = {
Expand Down
21 changes: 18 additions & 3 deletions packages/sveltekit/src/vite/sourceMaps.ts
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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:
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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 {
Expand Down
Loading