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

Add lifecycleHook in vue plugin #1053

Closed
wants to merge 5 commits into from
Closed

Add lifecycleHook in vue plugin #1053

wants to merge 5 commits into from

Conversation

ankurk91
Copy link
Contributor

@ankurk91 ankurk91 commented Sep 22, 2017

Vue.js v2.2.0 also captures errors in component lifecycle hooks
https://vuejs.org/v2/api/#errorHandler

Before submitting a pull request, please verify the following:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (npm test).

Vue.js v2.2.0 also captures errors in component lifecycle hooks
https://vuejs.org/v2/api/#errorHandler
@ankurk91
Copy link
Contributor Author

ankurk91 commented Sep 22, 2017

Whoops, look like one of test failed.
Let me know how to mock the lifecycle hook.

@kamilogorek
Copy link
Contributor

Hey @ankurk91, thanks for contributing, appreciate it.

In order to fix the test, you need to call errorHandler with additional 3rd argument, just as Vue.js does, see https://github.com/getsentry/raven-js/blob/master/test/plugins/vue.test.js#L22 and https://github.com/getsentry/raven-js/blob/master/test/plugins/vue.test.js#L53

Three things should be changed:

  • could you please remove this redundant object on line 31 here? https://github.com/getsentry/raven-js/blob/master/test/plugins/vue.test.js#L31 (it's not used anyway, so you can just delete it)
  • make your lifeCycleHook: info optional, as this is how Vue.js handles it (info may not be passed, so we don't want to create undefined lifecycleHook). Just check if info is passed, and only then add this property to extra attributes
  • rename this field to lifecycleHook, this is how Vue and Angular spell it (lifecycle, not life cycle – although both are correct in english)
  • write separate, 3rd test for this case, to make sure that it's indeed sent only when info is defined

@ankurk91 ankurk91 changed the title Add lifeCycleHook in vue plugin Add lifecycleHook in vue plugin Sep 22, 2017
@ankurk91
Copy link
Contributor Author

Now one of the task is failing that is not related to this PR

Running "config:ci" task
No SAUCE_USERNAME env variable defined.
No SAUCE_ACCESS_KEY env variable defined.

@kamilogorek
Copy link
Contributor

It's my silly config mistake, tests are passing just fine :)
Could you please fix L25:35 indentation please? (it should be taken care of by prettier, not sure why it wasn't 🤔).

@ankurk91
Copy link
Contributor Author

Pre-commit hook was causing indentation issues, it should be fixed now.

@kamilogorek
Copy link
Contributor

Had to rebase it locally and pass through prettier for code formatting.
Merged it by hand – 4bd15d4

Thanks!

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.

2 participants