Experiments with mocks for SDK and SwiftUI #16
Conversation
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. 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. - 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/
…at'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).
| } | ||
| } | ||
|
|
||
| final class MockMessages: @unchecked Sendable, Messages, ObservableObject { |
There was a problem hiding this comment.
Why the @unchecked? That’s used to tell the compiler "trust me, I’ve made sure that this is Sendable, you don't need to check it" and normally implies some synchronisation mechanism of your own. It doesn't seem like it should be necessary here.
There was a problem hiding this comment.
It forced by compiler. Without @unchecked it gives an error:
Stored property '_log' of 'Sendable'-conforming class 'MockMessages' is mutable
If I change MockMessages to be a struct, then it's impossible to use it with ObservableObject protocol, because it's a class protocol.
There was a problem hiding this comment.
But the compiler is warning you about a genuine data race; suppressing it by doing @unchecked Sendable whilst not addressing the data race isn't the right thing to do.
| } | ||
|
|
||
| // compiler doesn't allow to use `room` property here, needs to think about proper initialization | ||
| @StateObject private var messagesClient = try! MockChatClient.shared.rooms.get(roomID: "Demo Room", options: .init()).messages as! MockMessages |
There was a problem hiding this comment.
Why the as! MockMessages? This means that we’re no longer just using the API of the SDK (i.e. the Messages protocol).
There was a problem hiding this comment.
It'd be good if we could stick to using the modern SwiftUI state management APIs introduced in iOS 17 (i.e. @State and @Observable); I think they are simpler to understand.
| let clientID: String | ||
| let roomID: String | ||
|
|
||
| @Published var log = [Message]() |
There was a problem hiding this comment.
What's the purpose of this and the ObservableObject conformance? It doesn't seem related to the SDK’s API.
|
(I know that this is a WIP but I’ve put some comments in here since I took a look after you mentioned it in #9.) |
I've meant that subscriptions are not iterable by design, they are event-based. They don't have an internal array to iterate through. Yes, I do iterate it in for await loop, but it's something I've found strange to be doing. But maybe it's just me.
just give it the most obvious name
Shouldn't it return the same as
Yes, you're right, it's SwiftUI only. Playing with it I forgot that the adoption rate of it is still low. Thanks for #17
The idea was that
Will look into that. |
But this does not currently fall within the functionality that the SDK offers, so why are you trying to add this functionality into the |
|
As mentioned the other day, I think it would be best if for #4 we simply created a mock implementation of the protocols, without trying to build an example app around them. |
…Sendable` to fit the `mockAsyncSequence` variable for avoiding of using `as!`.
|
I'm closing this in favor of #34 |
Research for #4