[ECO-4920] Define the public API of the SDK #9
Conversation
lawrence-forooghian
force-pushed
the
7-public-api
branch
2 times, most recently
from
ba5366b to
77dbbfb
Compare
lawrence-forooghian
force-pushed
the
7-public-api
branch
from
77dbbfb to
e1e4382
Compare
lawrence-forooghian
force-pushed
the
7-public-api
branch
from
e1e4382 to
815b882
Compare
There was a problem hiding this comment.
LGTM, left a few questions.
Update after my experiments with #4 (posting here for visibility, it's a commit message from my experimental PR):
I've implemented the bare minimum of the mocks methods just to see what's happening on the SwiftUI side of things when messages are added to the view (generated or sent with the "Send" button). I'm not sure how useful the concept of AsyncSequence for the pure event based nature of a chat app is, but for the sake of trying modern things we can implement it.
The only thing I would argue is the name "Subscription". You usually don't iterate through it, but rather get an event from it. I would suggest something like MessageFetcher (watcher, poller, checker, loader etc..) and the appropriate method inside Messages protocol returning it should be like fetch, poll or whatever this object is doing. I've also made the object implementing Messages being ObservableObject and created a @StateObject reference to it in the SwiftUI view and it works perfectly (for sending) without all that additional async/await AsyncSequence stuff. For simplicity I just concatenate two arrays so they are displayed together, but it should be the single array of messages inside the Messages client.
Also I've found nested types inside MessageSubscription rather complicated approach and changed it to be a single type for both AsyncSequence and AsyncIteratorProtocol protocols (this is discussable, I didn't dive too deeply into that).
lawrence-forooghian
force-pushed
the
7-public-api
branch
from
815b882 to
7c4ed6d
Compare
|
I’ve put a few comments on #16 but will put a few here too.
What do you mean when you say that "you usually don't iterate through it"? In #16 you have a
Do you mean the current
I don't really understand what you're doing with that; are you saying that you think that instead of async sequences, the SDK should use stateful objects that are marked with
Are you referring to the fact that
I don't know enough about |
lawrence-forooghian
force-pushed
the
7-public-api
branch
3 times, most recently
from
94cb448 to
26d3f2d
Compare
Responded in #16 (comment) |
Based on JS repo at 0b4b0f8. I have not performed any review or critique of the JS API, and simply copied it and mildly adapted to Swift. There are some things that I’m unsure about regarding the usability and implementability of this Swift API. We’ll be able to validate the usability when we do #4, which will create a mock implementation of this API and then build the example app around the mock. Implementability we’ll discover as we try to build the SDK. This is just a first attempt and all of the decisions can be revisited. TypeScript-to-Swift decisions: - I’ve named the entry point class DefaultChatClient instead of their ChatClient; this is so that I can have a public protocol named ChatClient. - My `throws` annotations are based on the JS docstrings (or in a few cases my assumptions). The Rooms accessors that throw in JS if a feature is not enabled instead call fatalError in Swift, which is the idiomatic thing to do for programmer errors [1]. - Skipped copying the docstrings from JS to avoid churn; created #1 to do this later. Turned off missing_docs for now. - The listener pattern in JS is instead implemented by returning an AsyncSequence. This was partly because of Umair’s architecture thoughts [2] which — at least my reading of it — indicated a desire to use some sort of reactive API, and partly because I was curious to try it out, having not used it before. I believe that we do not need an equivalent of the `off*` / `unsubscribe*` methods since iterating over an AsyncSequence is a pull rather than a push. And I believe (but am still quite shaky about the details, so may be wrong) that there are AsyncSequence lifecycle events (e.g. end of iteration, task cancellation) that we can use to manage the underlying ably-cocoa listeners. And, I’m sure that there will be things we have to consider about how to make sure that we don’t have leaks of MessageSubscriptions which cause messages to start accumulating in buffer that the user forgot exists. - RoomOptionsDefaults in JS is instead implemented here by giving the *Options types a no-args initializer that populates the default values. - I’ve copied the logging interface more or less from JS (but with LogHandler a protocol so that we can have argument labels). Will think about something more idiomatic in #8. Swift decisions and thoughts: - My decision on what should be a protocol and what should be a concrete type was fairly arbitrary; I’ve made everything a protocol (for mockability) except structs that are basically just containers for data (but this line is blurry; for example, this might introduce issues for somebody who wants to be able to mock Message’s isBefore(_:) method). One downside of using protocols is that you can’t nest types inside them (this would be nice for e.g. related enums) but it’s alright. - I’ve annotated all of the protocols that feel like they represent some sort of client with AnyObject; I don’t have a great explanation of why but intuitively it felt right (seemed like they should be reference types). - Having not yet used Swift concurrency much, I didn’t have a good intuition for what things should be Sendable, so I’ve put it on pretty much everything. Similarly, I don’t have a good sense of what should be annotated as `async` (for some of this our hand will probably also end up being forced by the implementation). I also am not sure whether the `current` / `error` properties for connection and room status make sense in a world where most things are async (especially if the intention is that, for example, the user check `current` before deciding whether to call a certain method, and this method will throw an error if they get it wrong, but the state of the world might have changed since they checked it and that’s not their fault), but I’ve kept them for now. - Chose to use existential types when one protocol returns another (i.e. `-> any MyProtocol`) instead of associated types, because it’s more readable (you can’t use opaque types in a protocol declaration) and I don’t think that we need to worry about the performance implications. - I’ve deferred adding helpful protocol conformances (Equatable, Hashable, Codable etc) to #10. - I’ve turned on the explicit_acl SwiftLint rule, which forces us to add an access control modifier to all declarations. This makes it less likely that we forget to make something `public` instead of the default `internal`. Resolves #7. [1] https://www.douggregor.net/posts/swift-for-cxx-practitioners-error-handling/ [2] https://ably.atlassian.net/wiki/spaces/SDK/pages/3261202438/Swift+Architecture+Thoughts
lawrence-forooghian
force-pushed
the
7-public-api
branch
from
26d3f2d to
20e7f5f
Compare
|
Pushed a change to fix the type of |
lawrence-forooghian
force-pushed
the
7-public-api
branch
from
1089a3a to
20e7f5f
Compare
This is in line with an SDK divergence decision which is logged in [1]. People were not happy with the JS naming and hence we decided to change it to this. The intention is that this change will find its way back into the JS SDK. [1] https://ably.atlassian.net/wiki/spaces/CHA/pages/3307405320/SDK+Divergence+Decision+Log
WalkthroughThe changes encompass significant updates to the SDK's structure and public interface for chat functionalities in Swift. Key modifications include the introduction of new protocols, classes, and configurations to enhance code organization and testing capabilities. The removal of certain linting rules and the addition of contributions guidelines further refine the development process, ensuring a more coherent experience for developers interacting with the SDK. Changes
Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Outside diff range, codebase verification and nitpick comments (1)
Tests/AblyChatTests/AblyChatTests.swift (1)
6-6: Review theMockRealtimeimplementation for completeness.The
MockRealtimeclass lacks implementation for thedeviceproperty, which is crucial for simulating real-time behavior. Ensure that the mock aligns with the testing strategy by implementing necessary real-time behaviors.
Example/AblyChatExample/Mocks/MockRealtime.swiftTests/AblyChatTests/Mocks/MockRealtime.swiftAnalysis chain
Test setup updated to use
DefaultChatClient.The change to use
DefaultChatClientwithMockRealtimeandClientOptionsis appropriate for creating more controlled and isolated test scenarios. Ensure that this new setup aligns with the overall testing strategy and thatMockRealtimeis properly implemented to mimic the real-time behavior expected in tests.The changes are approved, but verify the alignment with the testing strategy.
Run the following script to verify the
MockRealtimeimplementation:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `MockRealtime` to ensure it mimics real-time behavior correctly. # Test: Search for the `MockRealtime` class implementation. Expect: Proper methods and properties to simulate real-time behavior. rg --type swift -A 10 $'class MockRealtime'Length of output: 1571
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (29)
- .swiftlint.yml (1 hunks)
- CONTRIBUTING.md (1 hunks)
- Example/AblyChatExample.xcodeproj/project.pbxproj (5 hunks)
- Example/AblyChatExample/ContentView.swift (1 hunks)
- Example/AblyChatExample/Mocks/MockRealtime.swift (1 hunks)
- Sources/AblyChat/.swiftformat (1 hunks)
- Sources/AblyChat/.swiftlint.yml (1 hunks)
- Sources/AblyChat/BufferingPolicy.swift (1 hunks)
- Sources/AblyChat/ChatClient.swift (1 hunks)
- Sources/AblyChat/Connection.swift (1 hunks)
- Sources/AblyChat/EmitsDiscontinuities.swift (1 hunks)
- Sources/AblyChat/Headers.swift (1 hunks)
- Sources/AblyChat/Logging.swift (1 hunks)
- Sources/AblyChat/Message.swift (1 hunks)
- Sources/AblyChat/Messages.swift (1 hunks)
- Sources/AblyChat/Metadata.swift (1 hunks)
- Sources/AblyChat/Occupancy.swift (1 hunks)
- Sources/AblyChat/PaginatedResult.swift (1 hunks)
- Sources/AblyChat/Presence.swift (1 hunks)
- Sources/AblyChat/Reaction.swift (1 hunks)
- Sources/AblyChat/Room.swift (1 hunks)
- Sources/AblyChat/RoomOptions.swift (1 hunks)
- Sources/AblyChat/RoomReactions.swift (1 hunks)
- Sources/AblyChat/RoomStatus.swift (1 hunks)
- Sources/AblyChat/Rooms.swift (1 hunks)
- Sources/AblyChat/Subscription.swift (1 hunks)
- Sources/AblyChat/Typing.swift (1 hunks)
- Tests/AblyChatTests/AblyChatTests.swift (1 hunks)
- Tests/AblyChatTests/Mocks/MockRealtime.swift (1 hunks)
Files skipped from review due to trivial changes (3)
- .swiftlint.yml
- Sources/AblyChat/.swiftformat
- Sources/AblyChat/.swiftlint.yml
Additional comments not posted (30)
Sources/AblyChat/EmitsDiscontinuities.swift (2)
3-5: Clarify the functionality ofsubscribeToDiscontinuities.The protocol
EmitsDiscontinuitiessuggests handling service interruptions, but it's not clear if thesubscribeToDiscontinuitiesmethod also handles successful event notifications or only errors. Clarifying this in the method documentation would enhance understanding and usability.
1-1: Dependency on Ably framework.The import statement for
Ablydirectly ties this protocol to the Ably real-time service framework. Confirming that this dependency aligns with the overall architecture and dependency management strategy of the SDK would be prudent.Sources/AblyChat/Metadata.swift (1)
2-2: Discuss the use ofany Sendablein Metadata.The choice of
any Sendablefor the dictionary values allows for a flexible and type-safe container for metadata. However, it's important to ensure that all potential types conform toSendableto maintain thread safety. This choice should be documented clearly to avoid misuse.Sources/AblyChat/Rooms.swift (1)
1-5: Discuss the use ofany Roomand protocol inheritance.The return type
any Roomin thegetmethod provides flexibility but could lead to type ambiguity and challenges in maintaining type safety. Clarifying the expected types and their usage would enhance the API's robustness. Additionally, inheriting from bothAnyObjectandSendableis an interesting choice, ensuring that the protocol is class-bound while also being safe for concurrent use. Confirming that this aligns with the SDK's concurrency model and architectural decisions would be beneficial.Sources/AblyChat/Logging.swift (1)
1-14: Logging capabilities introduced.The introduction of
LogHandlerprotocol andLogLevelenum enhances the SDK's debugging and monitoring capabilities. The use of a protocol allows for flexible implementation of different logging strategies, which is a good practice. Ensure that all logging levels are used appropriately throughout the SDK to provide meaningful and actionable logs.The changes are approved.
Sources/AblyChat/RoomReactions.swift (1)
1-11: Room reactions handling introduced.The introduction of the
RoomReactionsprotocol with async methods and a subscription model is a significant enhancement for interactive features in the SDK. Ensure that thesendandsubscribemethods are correctly implemented and that thechannelproperty is properly integrated with the rest of the SDK to handle real-time updates efficiently.The changes are approved.
Sources/AblyChat/BufferingPolicy.swift (1)
1-7: Well-documented and clear implementation ofBufferingPolicy.The enum is straightforward and aligns with Swift's concurrency model by conforming to
Sendable. The comments are informative, providing clarity on its purpose and relation toAsyncStream.The code changes are approved.
Sources/AblyChat/Headers.swift (1)
1-13: Well-structured and consistent with TypeScript counterpart.The enum
HeadersValueand the typealiasHeadersare well-defined. The decision to omitundefinedis justified given the context of Swift. The use ofSendableensures that the types are safe to use in concurrent environments.The code changes are approved.
Sources/AblyChat/PaginatedResult.swift (1)
1-11: Robust implementation ofPaginatedResultprotocol.The protocol is well-designed to handle paginated results, a common pattern in SDKs. The use of
SendableandAnyObjectensures thread safety and class-only conformance. The properties are clearly defined, and the TODO comment about unwrapping is linked to an ongoing discussion, which is a good practice for tracking enhancements.The code changes are approved.
Sources/AblyChat/Typing.swift (2)
3-8: Well-structured protocol for typing notifications.The
Typingprotocol is well-designed, incorporating modern Swift concurrency features and proper error handling. The use ofSendableensures thread safety, which is crucial in asynchronous environments.The design and implementation of the
Typingprotocol are approved.
11-16: Clear and concise struct for typing events.The
TypingEventstruct is straightforward and effectively encapsulates the data related to typing events. The use ofSendablehere also promotes safe concurrency practices.The design and implementation of the
TypingEventstruct are approved.Sources/AblyChat/Occupancy.swift (2)
3-6: Well-structured protocol for occupancy data.The
Occupancyprotocol is effectively designed, incorporating modern Swift concurrency features and proper error handling. The use ofSendableensures thread safety, crucial in asynchronous environments.The design and implementation of the
Occupancyprotocol are approved.
9-16: Clear and concise struct for occupancy events.The
OccupancyEventstruct is straightforward and effectively encapsulates the data related to occupancy in chat environments. The use ofSendablehere also promotes safe concurrency practices.The design and implementation of the
OccupancyEventstruct are approved.Example/AblyChatExample/ContentView.swift (1)
6-9: Appropriate use ofDefaultChatClientfor enhanced configurability and testing.The change to initialize
ablyChatClientwithDefaultChatClientusingMockRealtimeandClientOptionsis a sensible approach, likely aimed at improving testing capabilities and configuration flexibility. This setup allows for more granular control over the client behavior during development and testing phases.The change in the initialization of
ablyChatClientis approved.Sources/AblyChat/Reaction.swift (1)
3-22: Well-structuredReactiondata model.The
Reactionstruct is clearly defined and appropriately uses public access modifiers to ensure it is part of the SDK's public API. The use ofSendableconforms to Swift's concurrency model, which is a good practice for data types that might be shared across different threads.The structure and implementation of the
Reactionstruct are appropriate and follow Swift best practices.Sources/AblyChat/Message.swift (2)
6-14: Review ofMessagestruct definition and properties.The
Messagestruct is well-defined with clear properties that are essential for a chat message. The use ofpublicaccess modifier ensures that these properties are accessible as part of the SDK's public API. The struct also conforms toSendable, which is appropriate for ensuring thread safety in concurrent environments, a common scenario in chat applications.The struct definition and properties are correctly implemented and align with Swift's conventions for public APIs.
15-23: Review ofMessageinitializer.The initializer is comprehensive, initializing all properties of the
Messagestruct. This approach ensures that all properties are set upon creation of aMessageinstance, which is crucial for data integrity.The initializer is correctly implemented and follows Swift best practices for struct initializers.
Sources/AblyChat/RoomOptions.swift (4)
3-14: Review ofRoomOptionsstruct and its properties.The
RoomOptionsstruct is designed to be flexible, allowing optional configuration of various features like presence, typing, reactions, and occupancy. This design supports customizable room setups, which can cater to different use cases.The struct and its properties are well-implemented, providing necessary flexibility and clear separation of concerns within the SDK.
17-24: Review ofPresenceOptionsstruct.The
PresenceOptionsstruct provides straightforward options for presence functionality, with sensible defaults. This approach allows developers to easily configure presence features without needing to specify every option.The implementation is clear and follows best practices for optional boolean flags in Swift.
27-32: Review ofTypingOptionsstruct.The
TypingOptionsstruct allows for the configuration of typing indicators with a timeout. The default timeout of 10 seconds is a reasonable default that balances responsiveness with practicality.The struct is well-designed, with a clear and simple API for configuring typing behavior in chat rooms.
35-37: Review ofRoomReactionsOptionsandOccupancyOptionsstructs.Both
RoomReactionsOptionsandOccupancyOptionsare minimal structs, potentially designed to be expanded in the future. Currently, they serve as placeholders to encapsulate future settings related to reactions and occupancy.While these structs are minimal, they are correctly implemented and provide a foundation for future enhancements.
Also applies to: 39-41
Tests/AblyChatTests/Mocks/MockRealtime.swift (1)
4-41: Review ofMockRealtimeclass.The
MockRealtimeclass serves as a placeholder for mocking theARTRealtimeProtocol. Each method throws afatalError, indicating that they are not yet implemented. This approach is acceptable as a temporary measure, but it should be addressed soon to enable effective testing.The class structure and method stubs are appropriate for a mock implementation. However, ensure that proper mocking is implemented as planned in the linked GitHub issue.
+// TODO: Implement mocking methods as described in https://github.com/ably-labs/ably-chat-swift/issues/5Sources/AblyChat/ChatClient.swift (1)
3-9: Ensure robustness in theChatClientprotocol definition.The protocol defines essential properties for a chat client. Ensure that each property is necessary and appropriately typed to avoid future breaking changes.
Sources/AblyChat/Presence.swift (2)
3-17: Well-structured protocol with room for improvement inPresenceData.The
Presenceprotocol is well-defined, providing a comprehensive interface for presence management. However, the linked GitHub issue (#13) suggests that improvements toPresenceDataare being considered. It would be beneficial to keep an eye on this discussion to ensure that any changes are integrated smoothly into the protocol.
20-41: Well-designed struct with potential improvements forextras.
PresenceMemberis effectively designed to represent presence data, incorporating modern Swift concurrency features. The TODO comment (#13) regarding theextrastype suggests that there may be room for improvement in this area. Monitoring this issue could be beneficial to ensure that any enhancements are consistent with the rest of the SDK.Sources/AblyChat/Messages.swift (3)
3-7: Comprehensive protocol with a modern Swift approach.The
Messagesprotocol is well-structured, offering a clear interface for message management using modern Swift features. The use ofAsyncSequencefor message subscription, as discussed in previous comments, aligns with the desire to explore reactive APIs and should be evaluated for its practical benefits in real-world scenarios.
10-19: Well-designed struct for message sending parameters.
SendMessageParamsis effectively designed to encapsulate all necessary information for sending a message, ensuring type safety and clarity. This design supports easy maintenance and future enhancements.
22-38: Clear and flexible struct for message query options.
QueryOptionsis well-designed, offering clear and flexible options for customizing message queries. The use of an enum forResultOrderenhances clarity and maintainability. Consider adding more options in the future to increase flexibility, such as filtering based on message attributes.CONTRIBUTING.md (1)
28-30: Approval of Development Guidelines UpdateThe addition of guidelines to use protocols for describing SDK functionality and ensuring public memberwise initializers for structs is a strong move towards improving testability and mockability of the SDK. This aligns well with idiomatic Swift practices and enhances the developer experience.
The changes are approved as they contribute positively towards making the SDK more accessible and easier to test.
Example/AblyChatExample.xcodeproj/project.pbxproj (1)
10-10: Approval of Project Configuration UpdatesThe integration of
MockRealtime.swiftinto the project's build and organizational structure is executed correctly. The addition of this file under a new groupMocksis a strategic decision to enhance the modularity and clarity of the project structure, facilitating easier navigation and maintenance.The changes are approved as they enhance the project's organization and functionality, aligning with the objectives to improve testing capabilities.
Also applies to: 19-19, 40-47, 75-75, 166-166
jamienewcomb
changed the title
Resolves #7. See commit message for more details.
(Jira: ECO-4900)
Summary by CodeRabbit
New Features
MockRealtimeclass for testing purposes.DefaultChatClientfor chat functionality with mock support.BufferingPolicyenumeration for event handling.ChatClientprotocol andDefaultChatClientimplementation.Presenceprotocol.Reactionstructure for handling user reactions in chat.Roomprotocol.Documentation
CONTRIBUTING.mdwith new guidelines for mocking the SDK.Bug Fixes
Tests
DefaultChatClientfor improved test scenarios.