ref: convert SentryViewHierarchyIntegration to Swift #7157
Conversation
Convert SentryViewHierarchyIntegration from Objective-C to Swift, following the same pattern as SentryScreenshotIntegration. The Swift implementation uses dependency injection and the SwiftIntegration protocol. Changes: - Convert SentryViewHierarchyIntegration.m/.h to Swift - Add ViewHierarchyIntegrationProvider protocol for DI - Convert test helper from Objective-C to Swift using @_cdecl - Remove unused TestSentryViewHierarchyProvider.h header - Update tests to use Swift integration initialization pattern
|
@sentry review |
Semver Impact of This PR🟢 Patch (bug fixes) 📋 Changelog PreviewThis is how your changes will appear in the changelog. This PR will not appear in the changelog. 🤖 This preview updates automatically when you update the PR. |
|
Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 83bf9af | 1213.30 ms | 1234.18 ms | 20.89 ms |
| 013fd4d | 1216.02 ms | 1242.16 ms | 26.14 ms |
| 9f7ef2b | 1213.53 ms | 1250.23 ms | 36.70 ms |
| d29a425 | 1209.96 ms | 1239.00 ms | 29.04 ms |
| 2f4ddaa | 1227.26 ms | 1260.04 ms | 32.78 ms |
| adeec82 | 1220.43 ms | 1254.94 ms | 34.51 ms |
| 3bf0d3f | 1202.12 ms | 1237.23 ms | 35.11 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 83bf9af | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 013fd4d | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 9f7ef2b | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| d29a425 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 2f4ddaa | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| adeec82 | 24.14 KiB | 1.04 MiB | 1.02 MiB |
| 3bf0d3f | 24.14 KiB | 1.04 MiB | 1.02 MiB |
philprime
requested review from
itaybre,
noahsmartin and
philipphofmann
as code owners
Sources/Swift/Integrations/ViewHierarchy/SentryViewHierarchyIntegration.swift
Show resolved
Hide resolved
| if let client = SentrySDKInternal.currentHub().getClient() { | ||
| self.client = client | ||
| client.addAttachmentProcessor(self) | ||
| } |
There was a problem hiding this comment.
m: I know that this was already the same before, but maybe it makes even sense to return nil here if there is no client, because if the client is nil, the integration won't work.
There was a problem hiding this comment.
Yeah, not having any client doesn't make much sense right now
There was a problem hiding this comment.
As long as the getClient returns a nullable value, I need to do null handling. So the only two options are:
- Do it as is, the integration exists but it's not installed as an attachment processor
- Cancel the setup of the integration, returning nil from the initializer
itaybre
left a comment
itaybre
left a comment
There was a problem hiding this comment.
Looks, good, but got one question
| if let client = SentrySDKInternal.currentHub().getClient() { | ||
| self.client = client | ||
| client.addAttachmentProcessor(self) | ||
| } |
There was a problem hiding this comment.
Yeah, not having any client doesn't make much sense right now
|
|
||
| func uninstall() { | ||
| sentrycrash_setSaveViewHierarchy(nil) | ||
| client?.removeAttachmentProcessor(self) |
There was a problem hiding this comment.
l: As far as I know we only create a new client on startSDK, so I wouldn't expect this client reference to change to a different one if we use SentrySDKInternal.currentHub().getClient()?.removeAttachmentProcessor(self)
This would just avoid saving the client reference
There was a problem hiding this comment.
Yes, you're absolutely right. That's a main problem in our integration architecture right now. When the client or the hub changes, this breaks. We don't need to fix this in this PR. Thanks for pointing it out.
There was a problem hiding this comment.
I am not sure if I understand correctly. If the client never changes, I don't see a reason to dynamically fetch it via SentrySDKInternal.currentHub().getClient() and instead only have the weak reference?
There was a problem hiding this comment.
The client only changes if you restart the SDK. In that case, also the integration gets deallocated and recreated. So right now it's not a real problem. It was previously, when you could still manually bind a different hub or client to the static SentrySDK. So my bad, I think it's not a problem now, but it could easily become a problem in the future. We should pass in the client or hub as a dependency and not retrieve it via the Static API.
There was a problem hiding this comment.
We could dynamically fetch it, but then it wouldn't work if you have multiple hubs, and each hub has its own integrations. That's not a valid use case right now, but it could be in the future, for Swift Vapor, for example. The integrations shouldn't depend on the static API, but rather on a specific hub or a client. They shouldn't even know there is a static API, or you can have different hubs. Does it make sense now?
There was a problem hiding this comment.
So, I could inject a factory "getter" method via the dependencies, so the integration itself doesn't have to know where the client comes from, it just knows that it needs to be fetched every time without keeping a reference. Would that resolve your concerns?
There was a problem hiding this comment.
Yes, and I think we could then use this factory getter in all integrations that need to access the client, but we don't have to do that in this PR. We can also follow up in another PR.
…w-hierarchy-integration
📜 Description
Convert SentryViewHierarchyIntegration from Objective-C to Swift, following the same pattern as SentryScreenshotIntegration. The Swift implementation uses dependency injection and the SwiftIntegration protocol.
Changes:
💡 Motivation and Context
Closes #6853
💚 How did you test it?
#skip-changelog