-
-
Notifications
You must be signed in to change notification settings - Fork 372
ref: Move SentryIntegrationProtocol out of hybrid public #6911
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6911 +/- ##
=============================================
+ Coverage 84.832% 84.931% +0.099%
=============================================
Files 453 453
Lines 27611 27587 -24
Branches 12115 12112 -3
=============================================
+ Hits 23423 23430 +7
+ Misses 4142 4112 -30
+ Partials 46 45 -1
... and 15 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
c542395 to
f4500cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Non-modular header import breaks hybrid SDK builds
Removing SentryIntegrationProtocol.h from the _Hybrid module map while keeping SentryBaseIntegration.h in it creates a module dependency issue. SentryBaseIntegration.h imports and conforms to SentryIntegrationProtocol, but that header is no longer part of any module (not in _Hybrid, not in the umbrella header). This can cause "include of non-modular header inside framework module" errors when hybrid SDKs use modular imports like @import Sentry._Hybrid.
Sources/Resources/Sentry.modulemap#L18-L19
sentry-cocoa/Sources/Resources/Sentry.modulemap
Lines 18 to 19 in f4500cc
| header "SentryBaseIntegration.h" |
philipphofmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, if it breaks a hybrid SDK, we can revisit. It shouldn't as pointed out here #6862 (comment)
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b9ceffb | 1222.57 ms | 1247.96 ms | 25.39 ms |
| 8ad303c | 1220.02 ms | 1231.79 ms | 11.77 ms |
| 72a8746 | 1215.23 ms | 1259.22 ms | 43.99 ms |
| 480185d | 1220.50 ms | 1246.04 ms | 25.54 ms |
| 04f41a1 | 1215.82 ms | 1252.36 ms | 36.54 ms |
| 605fa27 | 1226.31 ms | 1251.35 ms | 25.05 ms |
| b13e93a | 1236.24 ms | 1247.33 ms | 11.08 ms |
| 67e8e3e | 1238.80 ms | 1266.02 ms | 27.22 ms |
| 06a9389 | 1245.96 ms | 1276.51 ms | 30.55 ms |
| fc6557e | 1226.40 ms | 1249.88 ms | 23.48 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| b9ceffb | 23.75 KiB | 969.07 KiB | 945.32 KiB |
| 8ad303c | 23.75 KiB | 879.24 KiB | 855.49 KiB |
| 72a8746 | 23.74 KiB | 976.79 KiB | 953.05 KiB |
| 480185d | 23.75 KiB | 913.42 KiB | 889.67 KiB |
| 04f41a1 | 24.14 KiB | 1.01 MiB | 1015.39 KiB |
| 605fa27 | 23.75 KiB | 908.03 KiB | 884.28 KiB |
| b13e93a | 23.75 KiB | 855.37 KiB | 831.62 KiB |
| 67e8e3e | 23.75 KiB | 919.88 KiB | 896.13 KiB |
| 06a9389 | 23.74 KiB | 995.48 KiB | 971.73 KiB |
| fc6557e | 23.75 KiB | 866.68 KiB | 842.93 KiB |
dadfd28 to
2a225b2
Compare
0d6a42d to
7614e22
Compare
7614e22 to
f9c8edc
Compare
philipphofmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
#skip-changelog
Closes #6912