feat(sveltekit): Auto-wrap load functions with proxy module
#7994
Conversation
size-limit report 📦
|
| 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)};` | ||
| ); | ||
| } |
There was a problem hiding this comment.
Honestly I really like that we just do template strings for this instead of typescript templates. Makes this so much simpler.
| * | ||
| * @returns `true` if we can wrap the given file, `false` otherwise | ||
| */ | ||
| export async function canWrapLoad(id: string, debug: boolean): Promise<boolean> { |
There was a problem hiding this comment.
I am very curious how this will work out. I think this is a very sane approach 👍
There was a problem hiding this comment.
Yeah me too, I guess we'll learn it soon enough 😅
|
|
||
| const DEFAULT_PLUGIN_OPTIONS: SentrySvelteKitPluginOptions = { | ||
| autoUploadSourceMaps: true, | ||
| autoInstrument: true, |
There was a problem hiding this comment.
Doesn't have to be now but we should think about adding an escape hatch for not auto instrumenting particular routes. I've seen users in nexts that found this useful (eg some people didn't want to instrument their auth routes).
There was a problem hiding this comment.
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 && console.log(`Skipping wrapping ${id} because it already contains Sentry code`); | ||
| } | ||
|
|
||
| const hasLoadDeclaration = /(const|let|var|function)\s+load\s*(=|\()/gm.test(codeWithoutComments); |
There was a problem hiding this comment.
l: What about the case where the function declaration is not load? I think it's fine, but maybe we leave a note in the docs about it (or even add a check).
const someThingElse = () => {
};
export {
load: someThingElse
}
There was a problem hiding this comment.
I thought about this, too but I think this isn't valid syntax: https://developer.mozilla.org/en-US/docs/web/javascript/reference/statements/export#syntax (🤞 )
The closest thing to this example would be
const load = () => {};
export {
load
}which should be covered by the regex
There was a problem hiding this comment.
Ah yeah sorry missed something in the snippet - you gotta use as for esm, but this still applies if I'm thinking about this correctly.
export {
someThingElse as load
}There was a problem hiding this comment.
Ahh damnit, I missed this case. I think we could adjust the regex to test for /as\s+load/ but I guess this has the potential to interfere with TypeScript's type assertions. However, in the worst case, this would produce a build warning so I think it's fine.
not perfect but better than nothing
Introduce auto-wrapping of `load` functions in * `+(page|layout).(ts|js)` files (universal loads) * `+(page|layout).server.(ts|js)` files (server-only loads) See PR description for further details on API and methodology
Finally something somewhat robust (at least more than modifying the AST):
This PR introduces auto-wrapping of
loadfunctions in+(page|layout).(ts|js)files (universal loads)+(page|layout).server.(ts|js)files (server-only loads)API
API wise, this is not a breaking change for users, as internally, we just add an additional plugin which is returned by the
sentrySvelteKitplugin factory function. However, users can add new options to the factory functions, to control auto-wrapping:Methodology
(Almost) everything auto-wrapping related happens in a new plugin.
The plugin works by returning wrapping code in the
loadfunction whenever a file is encountered in which aloadfunction should be wrapped. The wrapper imports the user's module but appends a query string to the import path of the original module. This makes the nextloadcall to not return the wrapper but the actual user code.Unfortunately, the query string makes it to the final source map, meaning that we need to clean up the source map when building. We already flatten the source map anyway, so IMO it's fine to also just remove the string at that time.
This works very well for prod builds. In dev mode, client-side loads are wrapped correctly but unfortunately, server-loads are not wrapped. I didn't figure out why this happens but i vote we go with this approach anyway as it's still much safer than AST manipulation.
closes #7940