-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat(vue): Expose VueIntegration
to initialize vue app later
#9180
Conversation
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.
We'll need to add docs for this too!
packages/vue/src/sdk.ts
Outdated
/** | ||
* Initialize Vue-specific error monitoring for a given Vue app. | ||
*/ | ||
export function initVueApp(app: Vue): void { |
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.
I wonder if we should pass in client as an optional arg.
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.
sure, why not! will adjust it
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.
I added an optional client
argument to be passed.
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.
This might come in very handy for the Astro SDK! looks good to me but I had a suggestion around the API. As I said, we can probably expand it further with another optional param.
packages/vue/src/sdk.ts
Outdated
/** | ||
* Initialize Vue-specific error monitoring for a given Vue app. | ||
*/ | ||
export function initVueApp(app: Vue, client?: BrowserClient): void { |
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.
I'm just wondering (we can add this later if we actually need it) but wdyt about optionally passing in the Vue-specific options that are used by vueInit
? I'm thinking about a case where users would initialize the Browser SDK (or that would be done for them e.g. in the Astro SDK) and they'd additionally pass in the Vue options here.
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.
Hmm maybe actually instead of passing the client optionally, we should pass the options directly? 🤔 as we don't really need the client in there, we just pick the options off...?
After talking about this with Lukas, I rewrote this to actually expose this as an integration. This is more in line with how we do this in other places. With this, you can lazy initialize this like this: Sentry.init({
integrations: (integration) => integration.id !== 'Vue',
});
// Later
Sentry.getCurrentHub().getClient()?.addIntegration(new Sentry.VueIntegration({ app })); Plus we also talked about the need to have a proper top level method to add an integration, (which we can do in a separate PR), it should just be: Sentry.addIntegration(...); in an ideal world. This also makes it easier to actually use |
initVueApp
method to initialize vue app laterVueIntegration
to initialize vue app later
This exposes a new
VueIntegration
from@sentry/vue
which can be used to initialize vue error tracking for a late-defined vue app.With this, you can initialize the SDK early, even if vue is not setup yet: