Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

ref: Remove currentHub from SentrySDK #1019

Merged
merged 1 commit into from
Mar 25, 2021
Merged

Conversation

philipphofmann
Copy link
Member

@philipphofmann philipphofmann commented Mar 25, 2021

📜 Description

Remove SentrySDK.currentHub and SentrySDK.setCurrentHub, which are only
needed internally and by hybrid SDKs. They are not intended to be public API.

This breaks Hybrid SDKs, but we are going to fix this with adding captureEnvelope and storeEnvelope to SentrySDK.m but not in the header. The hybrid SDKs can create a category to make these two methods accessible. We use this pattern for the test initializers already.

💡 Motivation and Context

See getsentry/develop#248 (comment)

💚 How did you test it?

CI.

📝 Checklist

  • I reviewed the submitted code
  • I added tests to verify the changes
  • I updated the CHANGELOG if needed
  • I updated the docs if needed
  • Review from the native team if needed
  • No breaking changes

🔮 Next steps

Remove SentrySDK.currentHub and SentrySDK.setCurrentHub, which are only
needed internally and by hybrid SDKs. They are not intended to be public API.
@philipphofmann philipphofmann added this to the 7.0.0 milestone Mar 25, 2021
@philipphofmann philipphofmann requested a review from a team March 25, 2021 11:03
@marandaneto
Copy link
Contributor

@philipphofmann what would be the alternative for SentrySDK.currentHub().capture(envelope: envelope) ? before removing, we need to guarantee that this is possible either via SentrySDK static class or elsewhere as Hybrid SDKs rely on it

@philipphofmann
Copy link
Member Author

@philipphofmann what would be the alternative for SentrySDK.currentHub().capture(envelope: envelope) ? before removing, we need to guarantee that this is possible either via SentrySDK static class or elsewhere as Hybrid SDKs rely on it

Sorry, I should have put this in the PRs description. This is the first step of this comment here: getsentry/develop#248 (comment). See point 2.

Copy link
Contributor

@marandaneto marandaneto left a comment

Choose a reason for hiding this comment

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

NICE, LGTM

@philipphofmann philipphofmann merged commit 1a6cfb7 into master Mar 25, 2021
@philipphofmann philipphofmann deleted the ref/remove-current-hub branch March 25, 2021 14:00
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants