Remove implicit Sentry initialization #62
Conversation
|
one change per commit please. and describe the change in commit message. also, describe the supposed new usage in readme. totally different changes should go to different pr. |
|
@glensc correct me if i am wrong, but this is only targeting a single change but closes multiple issues. i'm more than willing to rebuild this PR to incorporate the change here and update the readme, tests, etc. |
|
changing dependency to peer dependency can definitely be a separate commit and a PR. the other changes are also questionable and are already breaking. see the GitHub actions output. |
|
Hey hey! I did this blind but I can spend some time to fix the Typescript issues if you want 👍 Just looking to help out - any mutual efforts to collaborate are appreciated! |
|
seems you want this merged. I can help with merge if I understand the changes and they don't break things. those were my requirements. the current state is not acceptable for me to put my name on the merge. |
|
marking as draft. this is nowhere near complete. any of the errors that ci has reported have not been fixed. and as original author has written, they have not tested this. this might have moved faster if separate changes were put to different pull requests. |
| public constructor(options?: any, initializeSentry?: boolean = true) { | ||
| const validatedOptions = this.validateOptions(options || {}); | ||
|
|
||
| if (initializeSentry) { |
There was a problem hiding this comment.
what about having this set via options? also the what would get sentry return below?
the only way oi see this to work, if you pass an instance of Sentry to the constructor. and use that value in rest of the code.
|
I put something like this quickly together from my notes above: |
|
Extracted peerDependency change to separate PR |
Thanks for the project!
This is the best I could do without introducing a breaking change. I'd recommend removing Sentry initialization altogether in a future major release.
I haven't tested this
Fix #57 and #56