-
Notifications
You must be signed in to change notification settings - Fork 113
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: use new sentry sdk #30
Conversation
Nice. This will be included in the new v2.0 release, as NuxtJS recently published v2.0 themselves. After this has been merged, we can close #20 as well. Are you planning on adding tests yourself? |
@DiederikvandenB ye, i`l add something and test this week |
Sweet, looks like a seamless upgrade. |
@aldarund, is this ready to be merged in? The tests would be nice of course, but not completely essential. |
@DiederikvandenB i tried to add test, but wasnt able to properly mock sentry for testing purposes, so no tests. Didnt dig deeper into it cause of time as usual :) |
Thanks @aldarund. Don't worry about the tests ;). I'll think some more about the backwards compatibility. I was going to do a jump to version 2.0.0, so according to semver it'd be a breaking change anyway. Furthermore, a breaking change would make people aware the upgraded dependency, which is good I think. Having said that, breaking backwards compatibility while it's easily avoidable seems counter intuitive. Anyone want to weigh in? |
I have decided to follow the PR. Will release a breaking change 2.0 |
Sentry now have new packages. While old one will work, there wont be any new features added, only bugfixes.
https://forum.sentry.io/t/switching-to-sentry-javascript/4732
Not tested, help with testing welcome ;)