-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
feat(vue): Check if SDK is initialized before app is mounted #6227
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
Conversation
Thanks for being proactive and fixing this issue so fast @mydea 🥔 |
size-limit report 📦
|
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.
LGTM, as discussed, console.error
ing this is probably fine, although we might want to revisit it if we get requests.
I was thinking a little bit about downgrading this to a warning but in the end it boils down to the same question of using the logger or console. So no strong opinions on the level.
I guess we can "downgrade" it to a warning, really the important part is that people see it :D 👍 |
414dcac
to
332e1e8
Compare
FYI I tested this manually in a vue2 app, and it does not trigger the warning in any case. import Vue from 'vue';
import App from './App.vue';
import * as Sentry from "@sentry/vue";
import { BrowserTracing } from "@sentry/tracing";
Sentry.init({
Vue,
// ... etc
});
new Vue({
render: (h) => h(App),
}).$mount('#app'); or: new Vue({
render: (h) => h(App),
}).$mount('#app');
Sentry.init({
Vue,
// ... etc
}); But I'd say it is OK to only add the warning for vue 3. |
332e1e8
to
bf67f18
Compare
Update: I also "upgraded" another misconfiguration warning to |
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.
LGTM! Just had a last question but nothing blocking
"devDependencies": { | ||
"vue": "~3.2.41" | ||
}, | ||
"scripts": { |
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.
l: I'm probably missing something obvious but are we using the Vue dependency somewhere? AFICT, we're still importing the Vue
type from our type definition.
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 use it in the test now :)
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.
Ahh makes sense :D
This PR adds an error log when trying to mount the vue app BEFORE the sentry SDK is initialized.
I was thinking about if this should be
console.error
orlogger.error
, but since this can lead to silent failures/issues (e.g. sentry may simply not pick up some errors), I'd argue it's fine to actually always log them out - otherwise, chances are users will not notice anyhow.This uses somewhat internal stuff, but since we can literally check on
=== true
this should degrade very gracefully in that it otherwise simply doesn't log anything.Closes #6226
Elaboration
If you have code like this:
It will trigger a crash, but not be logged to sentry, in the case when the app is mounted before sentry is initialized.