Skip to content
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): Check if SDK is initialized before app is mounted #6227

Merged
merged 2 commits into from Nov 18, 2022

Conversation

mydea
Copy link
Member

@mydea mydea commented Nov 17, 2022

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 or logger.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:

// App.vue

<script setup>
// This starter template is using Vue 3 <script setup> SFCs
// Check out https://vuejs.org/api/sfc-script-setup.html#script-setup
import HelloWorld from './components/HelloWorld.vue'

window.doesNotExist();
</script>

It will trigger a crash, but not be logged to sentry, in the case when the app is mounted before sentry is initialized.

@mydea mydea requested review from Lms24 and a team November 17, 2022 14:55
@mydea mydea self-assigned this Nov 17, 2022
@mydea mydea requested review from lforst and removed request for a team November 17, 2022 14:55
@priscilawebdev
Copy link
Member

priscilawebdev commented Nov 17, 2022

Thanks for being proactive and fixing this issue so fast @mydea 🥔

@github-actions
Copy link
Contributor

github-actions bot commented Nov 17, 2022

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.52 KB (+0.08% 🔺)
@sentry/browser - ES5 CDN Bundle (minified) 60.37 KB (+0.07% 🔺)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.17 KB (+0.1% 🔺)
@sentry/browser - ES6 CDN Bundle (minified) 53.72 KB (+0.05% 🔺)
@sentry/browser - Webpack (gzipped + minified) 19.91 KB (+0.07% 🔺)
@sentry/browser - Webpack (minified) 65.18 KB (+0.12% 🔺)
@sentry/react - Webpack (gzipped + minified) 19.94 KB (+0.12% 🔺)
@sentry/nextjs Client - Webpack (gzipped + minified) 45.89 KB (+0.04% 🔺)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.33 KB (+0.05% 🔺)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.74 KB (+0.07% 🔺)

Copy link
Member

@Lms24 Lms24 left a 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.erroring 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.

@mydea
Copy link
Member Author

mydea commented Nov 18, 2022

I guess we can "downgrade" it to a warning, really the important part is that people see it :D 👍

@mydea
Copy link
Member Author

mydea commented Nov 18, 2022

FYI I tested this manually in a vue2 app, and it does not trigger the warning in any case.
Actually, in vue 2, an error in component definition will never be logged to Sentry, no matter the init order:

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.

@mydea
Copy link
Member Author

mydea commented Nov 18, 2022

Update: I also "upgraded" another misconfiguration warning to console.warn, and added a test for that as well.

Copy link
Member

@Lms24 Lms24 left a 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

Comment on lines +28 to 31
"devDependencies": {
"vue": "~3.2.41"
},
"scripts": {
Copy link
Member

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.

Copy link
Member Author

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 :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh makes sense :D

@mydea mydea merged commit fccca98 into master Nov 18, 2022
@mydea mydea deleted the fn/vue-check-mounted branch November 18, 2022 09:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Log warning if Vue app is mounted before Sentry.init is called
3 participants