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

fix: Make sure that error is present before logging it in Vue #3183

Merged
merged 1 commit into from
Jan 21, 2021

Conversation

kamilogorek
Copy link
Contributor

Fixes #3178

@kamilogorek kamilogorek requested a review from a team January 19, 2021 14:18
@github-actions
Copy link
Contributor

size-limit report

Path Size
@sentry/browser - CDN Bundle (gzipped) 20.19 KB (-0.01% 🔽)
@sentry/browser - Webpack 21.05 KB (0%)
@sentry/react - Webpack 21.08 KB (0%)
@sentry/browser + @sentry/tracing - CDN Bundle (gzipped) 27.34 KB (-0.01% 🔽)

@@ -426,7 +426,7 @@ export class Vue implements Integration {

if (this._options.logErrors) {
if (this._options.Vue.util) {
this._options.Vue.util.warn(`Error in ${info}: "${error.toString()}"`, vm);
this._options.Vue.util.warn(`Error in ${info}: "${error && error.toString()}"`, vm);
Copy link
Member

Choose a reason for hiding this comment

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

I don't know Vue, but will there ever be info if there is no error? What about vm?

In any case, it might be nice to provide the user with the information that the error is null rather than just now showing anything. That way, they know it's not the reporting that's broken.

Suggested change
this._options.Vue.util.warn(`Error in ${info}: "${error && error.toString()}"`, vm);
this._options.Vue.util.warn(`Error in ${info}: "${error && error.toString() || 'null'}"`, vm);

Copy link
Contributor

Choose a reason for hiding this comment

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

Error in foo: null

Looks strange to me, why would there be a warning in the first place.

Copy link
Member

@lobsterkatie lobsterkatie Jan 20, 2021

Choose a reason for hiding this comment

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

Worse than Error in foo:, though? The dangling colon feels wrong to me.

Maybe Error in foo: <null> is better?

@rhcarvalho
Copy link
Contributor

Since this was a bug, could we add a corresponding test case? Ok to do it in a follow up if you want to ship this timely.

@kamilogorek kamilogorek merged commit 090dd10 into master Jan 21, 2021
@kamilogorek kamilogorek deleted the vue-log-error branch January 21, 2021 13:17
@kamilogorek
Copy link
Contributor Author

Will do that @rhcarvalho, just wanted to make sure it's unblocked for some people.

This was referenced Mar 7, 2021
This was referenced Mar 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

vue.js - TypeError: Cannot read property 'toString' of undefined
3 participants