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
fix(sveltekit): Avoid double-wrapping load functions #8094
Conversation
size-limit report 📦
|
const patchedEvent: PatchedLoadEvent = { | ||
...event, | ||
__sentry_wrapped__: true, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
h: Can we use addNonEnumerableProperty
and mutate the original event? This way we can just do wrappingTarget.apply(thisArg, args)
like before, which is more safe if more args are added in the future.
const patchedEvent: PatchedLoadEvent = { | |
...event, | |
__sentry_wrapped__: true, | |
}; | |
addNonEnumerableProperty(event, '__sentry_wrapped__', true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah we can do that easily for the server-side, good call! On the client side, we have to patch the event for the fetch instrumentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 1d1dd92
As mentioned in the discussion in https://github.com/getsentry/sentry-javascript/pull/8083/files#r1188707540, applying the
wrap(server)?LoadWithSentry
wrappers multiple times shouldn't lead to double wrapping but instead we should detect if we already wrapped a load function and no-op in this case. This PR patches the respective load events with a flag to detect double wrapping.Double wrapping can occur, for instance, if users already applied the load wrapper function to their code but also use auto-wrapping. With this change, we'll still apply the wrapper twice but the second wrapper will simply no-op.
Once this is merged, we can adjust the
canWrapLoad
auto-instrumentation check to no longer bail out of auto-wrapping if Sentry code was detected.