Skip to content

ref: Expose Raven's constructor publicly. Kinda. #1272

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

Merged
merged 1 commit into from
Mar 23, 2018

Conversation

kamilogorek
Copy link
Contributor

Resolves: #1269

@kamilogorek kamilogorek changed the title ref: Expose Raven's constructor publicly ref: Expose Raven's constructor publicly. Kinda. Mar 22, 2018
@kamilogorek kamilogorek requested a review from a team March 22, 2018 15:20
Copy link
Contributor

@MaxBittker MaxBittker left a comment

Choose a reason for hiding this comment

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

would it be worth prefixing this with _ to act as more warning? The comment is good but the name isn't "scary" on it's own like "dangerouslysetinnerhtml" or something

@kamilogorek kamilogorek merged commit 49a2fa2 into master Mar 23, 2018
@kamilogorek kamilogorek deleted the expose-constructor branch March 23, 2018 09:07
@kamilogorek
Copy link
Contributor Author

@MaxBittker I wanted it to be in line with node's implementation for now. It'll be updated and taken care of correctly in v4 :)

@zivl
Copy link
Contributor

zivl commented Mar 24, 2018

@kamilogorek thanks for the super-fast delivery!
Cheers!

@dan-kez
Copy link

dan-kez commented Apr 7, 2018

Quick question on this documentation @kamilogorek

Rather than

someAppReporter('__DSN__', {
  ...config goes here
});

shouldn't it be:

someAppReporter.config('__DSN__', {
  ...config goes here
}).install();

@kamilogorek
Copy link
Contributor Author

kamilogorek commented Apr 9, 2018

@dmk255 ah! Yes, thank you. I forgot the config. Although it shouldn't use install as it'd then literally install all the error/promise handlers. We want to avoid it in this scenario, as multiple reporters are meant to be used manually via captureException and captureMessage.

Updated: f3b3500

@dan-kez
Copy link

dan-kez commented Apr 10, 2018

@kamilogorek Quick follow up on that though. I haven't had that issue in my local testing with multiple install calls. That said, I am using webpack code chunking and only call install in separate chunks. This so far has allowed me to have multiple sentry reporting pipelines for chunked pages that each have their own redux stores.

Is there is a subtly that I'm missing or am I fine so long as I ensure that install is called from separate files (chunks)?

@kamilogorek
Copy link
Contributor Author

kamilogorek commented Apr 10, 2018

What install does is add all the handlers to the current page - https://github.com/getsentry/raven-js/blob/master/src/raven.js#L239-L266
What that implies, is that when you call install twice, you'll get multiple handlers and errors will be caught by both of them. Unless the previous one will be removed, which can be done using uninstall OR page will be reloaded and all the handers wiped out.

Everything I said is true only when you actually rely on handlers and don't just use capture methods yourself, as those are created per-instance.

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.

4 participants