Move presence property back to ARTRealtime and change its type back to concrete
#2015
Conversation
That change, which moved the `presence` property from the ARTRealtimeChannel class to ARTRealtimeChannelProtocol, changed the type of the property from ARTRealtimePresence to id<ARTRealtimePresenceProtocol>. This is a breaking change for anybody who was relying on the property returning the concrete type, one such example being our Asset Tracking SDK. So, we undo it. The change made in 26d9bf7 was triggered by us noticing, whilst working on the Chat SDK — which does its best to avoid referring to ably-cocoa's concrete types — that when you have an ably-cocoa concrete type ConcreteA which implements a protocol ProtocolA, and where ConcreteA has a public method or property `someMethod` which returns an instance of another concrete type ConcreteB which happens to implement a protocol ConcreteB, then `someMethod` is always defined directly on ConcreteA, not on ProtocolA. We wondered "why can't these methods be on ProtocolA instead?", hence 26d9bf7. But the first reason is that we can't because in order to do this it makes sense for you to also change the the return type of `someMethod` from ConcreteB to ProtocolB, which is a breaking change as we've seen here. But also I'm not sure whether it'd work even if we accepted the breaking change; for example, I don't know if you can mark a protocol as Sendable in Objective-C (haven't looked into it since we currently have no intention of making the breaking change, but one to investigate if we consider it in the future). Resolves #2003.
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 SwiftLint (0.57.0)Test/Tests/RealtimeClientChannelTests.swift[ Test/Tests/RealtimeClientPresenceTests.swift[ WalkthroughThe pull request introduces changes to the Ably Cocoa SDK's presence management, primarily focusing on refactoring how presence is accessed and handled across multiple files. The modifications involve changing the Changes
Assessment against linked issues
Possibly related issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (8)
✅ Files skipped from review due to trivial changes (1)
🧰 Additional context used📓 Learnings (4)Source/ARTRealtimeChannels.m (1)Source/PrivateHeaders/Ably/ARTRealtimeChannel+Private.h (1)Source/include/Ably/ARTRealtimeChannel.h (2)Source/ARTRealtimeChannel.m (1)⏰ Context from checks skipped due to timeout of 90000ms (6)
🔇 Additional comments (11)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
This reverts 26d9bf7.
That change, which moved the
presenceproperty from theARTRealtimeChannelclass toARTRealtimeChannelProtocol,changed the type of the property fromARTRealtimePresencetoid<ARTRealtimePresenceProtocol>. This is a breaking change for anybody who was relying on the property returning the concrete type, one such example being our Asset Tracking SDK. So, we undo it.The change made in 26d9bf7 was triggered by us noticing, whilst working on the Chat SDK — which does its best to avoid referring to ably-cocoa's concrete types — that when you have an ably-cocoa concrete type
ConcreteAwhich implements a protocolProtocolA, and whereConcreteAhas a public method or propertysomeMethodwhich returns an instance of another concrete typeConcreteBwhich happens to implement a protocolConcreteB, thensomeMethodis always defined directly onConcreteA, not onProtocolA. We wondered "why can't these methods be onProtocolAinstead?", hence 26d9bf7. But the first reason is that we can't because in order to do this it makes sense for you to also change the the return type ofsomeMethodfromConcreteBtoProtocolB, which is a breaking change as we've seen here. But also I'm not sure whether it'd work even if we accepted the breaking change; for example, I don't know if you can mark a protocol asSendablein Objective-C (haven't looked into it since we currently have no intention of making the breaking change, but one to investigate if we consider it in the future).Resolves #2003.
Summary by CodeRabbit
Code Refactoring
internalPresencetopresenceacross multiple filesType Changes
presenceproperty return type from protocol to concrete class inARTRealtimeChannelTest Updates