Send/Receive message draft #45

umair-ably · 2024-09-17T13:52:27Z

@lawrence-forooghian - I was wrong in that the macOS and iOS features diverged... I'm actually targeting iOS 16 but not macOS 16 hence the need to check available OS version... ChatAPI is the class to check (where I'm passing in a Protocol as a parameter instead of a concrete implementation)

Will merge main and resolve conflicts but this draft is good enough to get a gist of what is going on. I'll also refactor some of the access given to certain classes/structs, as well as improve general organisation

https://ably.atlassian.net/browse/ECO-4942

coderabbitai · 2024-09-17T13:52:36Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

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?

❤️ Share

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>, please review it.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Other keywords and placeholders

Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

lawrence-forooghian · 2024-09-17T14:45:56Z

@umair-ably what was your aim in using existential types (i.e. the any Foo<T> which is giving you the OS support issue)? Is there a reason you avoided just using the concrete types i.e. something like 71270cf?

lawrence-forooghian · 2024-09-17T14:49:14Z

Also, just to check — are you intending to add tests?

umair-ably · 2024-09-17T16:23:42Z

@umair-ably what was your aim in using existential types (i.e. the any Foo<T> which is giving you the OS support issue)? Is there a reason you avoided just using the concrete types i.e. something like 71270cf?

I started off with a generic implementation hence the protocol itself being passed in. I didn't want the base network "layer" to know about any concrete types in case we add additional implementations of the protocol later. But on second thoughts (and from seeing your solution), I'd already implemented the concrete type to also be fairly generic so I achieve my initial aim too - I just became blind to tying all of it together from staring at this too long 😅

Thanks for this - it perfectly solves the problem!

umair-ably · 2024-09-17T16:25:45Z

Also, just to check — are you intending to add tests?

Yeah for sure, ideally I want to put the completed implementation of the message spec in this PR... hoping you can review this first to point out any obvious errors/deviations/unnecessary public api changes, etc, and then it'll be a good base to finish the rest off

lawrence-forooghian

Added a few general thoughts, as requested. Some of the review is quite superficial, will look in more detail at the final one.

Also, please could you make it clear where the behaviour implemented here has come from, to aid with review now and to help our future selves understand it better. My suggestion would be to make sure that:

the code includes spec references, where they exist (I know that the code that I’m looking at now predates the existence of the spec)
code that's based on the implementation in the JS SDK should state this and should state the specific version (i.e. commit hash) of the JS SDK that it's based on – either do this at the place where the behaviour is implemented, or in commit message, whichever seems most appropriate

lawrence-forooghian · 2024-09-23T19:39:09Z

Example/AblyChatExample/ContentView.swift

@@ -1,24 +1,160 @@
-import AblyChat
+//


I assume all this stuff is going to get replaced by what Marat’s doing in #34?

lawrence-forooghian · 2024-09-23T19:42:56Z

Sources/AblyChat/Channel.swift

@@ -0,0 +1,22 @@
+import Ably
+
+func getChannel(_ name: String, realtime: RealtimeClient, opts: ARTRealtimeChannelOptions? = nil) -> any RealtimeChannelProtocol {


This could be an internal protocol extension on RealtimeProtocol

lawrence-forooghian · 2024-09-23T19:43:11Z

Sources/AblyChat/Channel.swift

+    }
+
+    resolvedOptions.params = opts?.params?.merging(
+        DEFAULT_CHANNEL_OPTIONS.params ?? [:],


Why capitalised?

lawrence-forooghian · 2024-09-23T19:45:36Z

Sources/AblyChat/ChatAPI.swift

+
+    func getMessages(roomId: String, params: QueryOptions) async throws -> any PaginatedResult<Message> {
+        let endpoint = "/chat/v1/rooms/\(roomId)/messages"
+        let response: any PaginatedResult<Message> = try await makeAuthorizedPaginatedRequest(endpoint, params: params.toDictionary())


Can't this directly be returned?

lawrence-forooghian · 2024-09-23T19:47:59Z

Sources/AblyChat/ChatAPI.swift

+        }
+
+        let response: SendMessageResponse = try await makeAuthorizedRequest(endpoint, method: "POST", body: body)
+        let timeIntervalInSeconds = TimeInterval(integerLiteral: response.createdAt) / 1000


It doesn't make sense to initialise a TimeInterval with a number of milliseconds — TimeInterval always represents seconds. Also this usage of the TimeInterval(integerLiteral:) initializer looks a bit out of place here; I'd suggest removing it.

lawrence-forooghian · 2024-09-23T20:38:25Z

Sources/AblyChat/Metadata.swift

+import Foundation
+
+// Define protocol for types that are Sendable and Encodable
+public protocol SendableEncodable: Sendable, Codable, Hashable {}


Is there a need for this? The name is particularly confusing because it doesn't mention Hashable.

lawrence-forooghian · 2024-09-23T20:39:01Z

Sources/AblyChat/Metadata.swift

+public protocol SendableEncodable: Sendable, Codable, Hashable {}
+
+// Example conforming type
+public struct MetadataValue: SendableEncodable {


What's the purpose of this new public type?

lawrence-forooghian · 2024-09-23T20:40:01Z

Sources/AblyChat/Timeserial.swift

+}
+
+// DefaultTimeserial Class
+final class DefaultTimeserial: Timeserial {


Why a class? Looks like something struct-like to me. Does it need reference semantics?

lawrence-forooghian · 2024-09-23T20:42:01Z

Sources/AblyChat/Timeserial.swift

+    }
+
+    // Convert to String
+    func toString() -> String {


This is a function which gets called automatically in JS. No such thing happens in Swift so this function doesn't really serve any purpose.

lawrence-forooghian · 2024-09-23T20:42:40Z

Sources/AblyChat/Version.swift

+// Update this when you release a new version
+
+// Version information
+public let VERSION = "0.1.0"


These capitalised constants aren’t very Swift-like; they could just be lets` with camel-cased names.

lawrence-forooghian · 2024-09-23T20:47:49Z

Sources/AblyChat/Room.swift

        self.realtime = realtime
        self.roomID = roomID
        self.options = options
+        self.chatAPI = chatAPI
+
+        self._messages = await DefaultMessages(


Why await?

@maratal for some reason I can't reply to #45 (comment) so I’ll do it here ("I agree that creating initializers that must be called with await should be avoided.")

I wasn’t saying that it should necessarily be avoided (e.g. in #53 I’m making the room lifecycle manager initializer async because it needs to perform some async setup work before it can be used); I was just curious what the need was here.

# Conflicts: # Package@swift-6.swift # Sources/AblyChat/ChatClient.swift # Sources/AblyChat/Room.swift # Sources/AblyChat/Rooms.swift Small refactor and start documenting spec points

lawrence-forooghian · 2024-09-27T12:59:14Z

Before removing from draft please could you also tidy up the commits; i.e. in addition to what I mentioned here also make sure to remove the merge commit and to incorporate linting fixes into the commit that introduced the errors. Thanks!

maratal

Changes in subscribe method prototype should be addressed as well as other minor suggestions.

maratal · 2024-09-29T14:55:18Z

Sources/AblyChat/ChatAPI.swift

+
+    // (CHA-M6) Messages should be queryable from a paginated REST API.
+    public func getMessages(roomId: String, params: QueryOptions) async throws -> any PaginatedResult<Message> {
+        let endpoint = "/chat/v1/rooms/\(roomId)/messages"


This should not be constructed from hardcoded strings. Ditto elsewhere.

maratal · 2024-09-29T15:01:09Z

Sources/AblyChat/ChatAPI.swift

+        let response: SendMessageResponse = try await makeAuthorizedRequest(endpoint, method: "POST", body: body)
+
+        // response.createdAt is in milliseconds, convert it to seconds
+        let createdAtInSeconds = TimeInterval(integerLiteral: response.createdAt) / 1000


I think it's TimeInterval(response.createdAt / 1000), TimeInterval is just a typealias for Double.

maratal · 2024-09-29T15:12:02Z

Sources/AblyChat/DefaultMessages.swift

+import Ably
+
+// Typealias for the timeserial used to sync message subscriptions with. This is a string representation of a timestamp.
+private typealias FromSerial = String


Why not TimeSerial then? FromSerial gives no understanding what this type is for.

maratal · 2024-09-29T15:18:13Z

Sources/AblyChat/DefaultMessages.swift

+    private let chatAPI: ChatAPI
+    private let clientID: String
+    private var listenerSubscriptionPoints: MessageListeners = [:]
+    private let realtime: RealtimeClient


I think ChatAPI should absorb all the functionality of the RealtimeClient, so you could use only ChatAPI elsewhere.

maratal · 2024-09-29T16:16:50Z

Sources/AblyChat/DefaultMessages.swift

+        self.clientID = clientID
+        self.realtime = realtime
+
+        Task {


I agree that creating initializers that must be called with await should be avoided.

maratal · 2024-09-29T16:56:16Z

Sources/AblyChat/Room.swift

@@ -20,23 +20,34 @@ public protocol Room: AnyObject, Sendable {
 internal actor DefaultRoom: Room {
    internal nonisolated let roomID: String
    internal nonisolated let options: RoomOptions
+    private let chatAPI: ChatAPI
+
+    private let _messages: any Messages


Subjective, but I would avoid using _ as swift private vars name notation. Might as well be private let defaultMessages: DefaultMessages

maratal · 2024-09-29T16:57:20Z

Sources/AblyChat/Timeserial.swift

+import Foundation
+
+// Timeserial Protocol
+internal protocol Timeserial: Sendable {


I guess it falls under camel case notation here - TimeSerial

Timeserial is correct, apparently

maratal · 2024-09-29T16:58:52Z

Sources/AblyChat/Version.swift

+public let channelOptionsAgentString = "chat-ios/\(version)"
+
+// Default channel options
+public let defaultChannelOptions: ARTRealtimeChannelOptions = {


This would be better inside ARTRealtimeChannelOptions as a static var - ARTRealtimeChannelOptions.default

maratal · 2024-09-29T17:01:08Z

Sources/AblyChat/Version.swift

+public let version = "0.1.0"
+
+// Channel options agent string
+public let channelOptionsAgentString = "chat-ios/\(version)"


Shouldn't it include "ably" as prefix? Also "ios" will be added by cocoa. WDYT @lawrence-forooghian

I assume this is copied from the JS implementation; let's wait and see if @umair-ably adds a spec point reference

maratal · 2024-09-29T17:02:03Z

Sources/AblyChat/Version.swift

+// Update this when you release a new version
+
+// Version information
+public let version = "0.1.0"


Shouldn't it be in a config file?

- move getChannel inside the ChatAPI to avoid needing both Realtime and ChatAPI when initialising DefaultMessages

…il due to out of memory error

… work again

umair-ably added 2 commits September 13, 2024 12:27

wip for sending and receiving messages

3d74810

refactor + point to main ably-cocoa

262bf7b

umair-ably requested a review from lawrence-forooghian September 17, 2024 13:52

umair-ably changed the title ~~Ably chat demo wip~~ Send/Receive message draft Sep 17, 2024

lawrence-forooghian reviewed Sep 23, 2024

View reviewed changes

umair-ably added 2 commits September 25, 2024 03:06

Merge branch 'main' into ablyChatDemo-wip

99af84c

# Conflicts: # Package@swift-6.swift # Sources/AblyChat/ChatClient.swift # Sources/AblyChat/Room.swift # Sources/AblyChat/Rooms.swift Small refactor and start documenting spec points

fix lint

e5db832

maratal self-requested a review September 27, 2024 12:10

maratal requested changes Sep 29, 2024

View reviewed changes

umair-ably added 5 commits September 30, 2024 01:19

Spec complete for sending and receiving messages

325bb75

Merge branch 'main' into ablyChatDemo-wip

84af6a1

- remove redundant code around channelWithProperties

12b49cf

- move getChannel inside the ChatAPI to avoid needing both Realtime and ChatAPI when initialising DefaultMessages

Remove local swiftlint - checking if this is causing CI linting to fa…

7fc0577

…il due to out of memory error

fix linting issues - had to remove swiftlint plugin locally for CI to…

b357fed

… work again

		@@ -0,0 +1,22 @@
		import Ably

		func getChannel(_ name: String, realtime: RealtimeClient, opts: ARTRealtimeChannelOptions? = nil) -> any RealtimeChannelProtocol {

Send/Receive message draft #45

Are you sure you want to change the base?

Send/Receive message draft #45

Conversation

umair-ably commented Sep 17, 2024 • edited by jamienewcomb Loading

coderabbitai bot commented Sep 17, 2024 • edited Loading

Review skipped

Chat

CodeRabbit Commands (Invoked using PR comments)

Other keywords and placeholders

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

lawrence-forooghian commented Sep 17, 2024 • edited Loading

lawrence-forooghian commented Sep 17, 2024

umair-ably commented Sep 17, 2024

umair-ably commented Sep 17, 2024 • edited Loading

lawrence-forooghian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawrence-forooghian Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawrence-forooghian commented Sep 27, 2024

maratal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawrence-forooghian Sep 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

umair-ably commented Sep 17, 2024 •

edited by jamienewcomb

Loading

coderabbitai bot commented Sep 17, 2024 •

edited

Loading

CodeRabbit Configuration File (`.coderabbit.yaml`)

lawrence-forooghian commented Sep 17, 2024 •

edited

Loading

umair-ably commented Sep 17, 2024 •

edited

Loading

lawrence-forooghian Sep 23, 2024 •

edited

Loading

lawrence-forooghian Sep 30, 2024 •

edited

Loading