feat(sveltekit): Add Sentry handlers to handle and handleError hooks #259
feat(sveltekit): Add Sentry handlers to handle and handleError hooks #259Lms24 merged 4 commits intolms/sveltekit-supportfrom
handle and handleError hooks #259Conversation
|
lib/Steps/Integrations/SvelteKit.ts
Outdated
| } | ||
| foundHandleError = true; | ||
| yellow( | ||
| 'Cannot safely wrap an `export function handleError` declaration. Please wrap it manually with `handleErrorWithSentry`', |
There was a problem hiding this comment.
Why can't we wrap an export function handleError declaration?
Could we make the transform something like this?
export const handleError = Sentry.handleErrorWithSentry(function _handleError() {
// ...
});Little more complicated for Sentry.sentryHandle because of sequence, so I'm fine to log out a warning there.
There was a problem hiding this comment.
We can, but I'm not sure if there are more problems involved if we convert a function export to an arrow function export. For instance the differences with this in the arrow vs normal functions?
Happy to take another look at this. Overall, I just want to avoid doing even more AST modification than we're currently doing. But your proposed transformation looks more reasonable to me than what I came up with before for this case😅
As for handle, I'm just always adding sequence, regardless of if we're the only handler or if there are others. So I think the complexity shouldn't be much higher than for handleError
There was a problem hiding this comment.
I think if we keep the function declaration (with function _handleError) we should be fine - if this takes too much time though (more the 2 hours), it's probably not worth it.
There was a problem hiding this comment.
Turns out, it wasn't all too hard to get this working (albeit not pretty either). So I just did it in this PR 0a50651
AbhiPrasad
left a comment
There was a problem hiding this comment.
Let's merge this in - you can add the changes to wrapping function exports in another PR if you decide its worth the time - sorry for not LOGAFing.
753065f to
f37bfe4
Compare
6cbeba7 to
4194da9
Compare
All good! It's a valid concern! I'll take another look at it |
…oks (#259) Add our handlers to the `handle` and `handleError` hooks in the SveltKit hooks files. Uses magicast and advanced AST manipulation to merge user-defined handlers with ours.
This PR adds our handlers to the
handleandhandleErrorhooks in the SveltKit hooks files. I tested this with various cases of already existing, user-defined handlers.One limtiation:
This currently only works for arrow function exports (
export const handleError = () => {}). The Kit docs examples always show that handlers should be exported with arrow functions but it is possible to add normal function exports (export function handleError(){}). In this case we just bail out and ask users to apply our handler manually. I believe this is a rather edge-casey scenario, so IMO it's fine to just bail at the moment.#skip-changelog
ref #244