Chat room skeleton #19

lawrence-forooghian · 2024-08-19T14:54:32Z

Replacement for ECO-4901 so that we can discuss / link in the open. Text of that issue:

Awaiting on a spec from the Chat-JS team but current understanding:

The room lifecycle manager is the most complicated piece when accounting for the implicit attach/detach requirements. This task is responsible for making a simple attempt at attaching and detaching to rooms, without additional complexities uncovered in the spec. They will be tackled in a later task.

┆Issue is synchronized with this Jira Task by Unito

equatable conformances are for tests, but not sure if right thing to do

equatable conformances are for tests, but not sure if right thing to do note that I've had ot make RoomLifecycle.current async, this seems odd but not sure there's a better way if we want to make use of swift's built-in concurrency, also it highlights to callers that there can't be a definitive concept of 'current' which was indeed a concern i'd highlighted earlier but also had to make `onChange` async so that I could do some state management, that seems odder. is there a way to instead bake the async-ness into the sequence? things where testing framework makes things messy with concurrency: - `async let` with things like XCTUnwrap, XCAssertEqual - async operations with XCTAssertThrowsError I hope that once Xcode 16 is released we can instead use Swift Testing.

lawrence-forooghian · 2024-08-28T13:18:04Z

I misunderstood the scope of this task; I thought it was to implement Andy’s spec in ably/specification#200. That's not what it is. The idea is to implement the following:

Rooms.get to return a room
and then Room.attach() will then attempt to attach to all of the channels listed in https://ably.atlassian.net/wiki/spaces/CHA/pages/3083501593/CHADR-025+Chat+Permissions+and+Implicit+Permissions+for+Realtime+Channels, succeeding if and only if all of these calls succeed
ditto for Room.detach(), detaching from the channels
emit ATTACHED and DETACHED room status updates if attach / detach method succeeds, respectively

lawrence-forooghian · 2024-08-28T13:32:45Z

Also we won't implement this in the example app as part of this task; will revisit at start of next sprint where hopefully the skeleton of the example app will have been built out.

This is based on [1] at aa7455d. It’s generated some questions, which I’ve asked on that PR. I started implementing this as part of #19, before realising that implementing the spec is not the aim of that task. So, putting this work on hold until we pick it up again in #28. So far, only the ATTACH operation is implemented. Note that I've had to make RoomLifecycle.{ current, error } async, this seems odd but not sure there's a better way if we want to make use of Swift's built-in concurrency. Also it highlights to callers that in a multi-threaded environment there can't be a definitive concept of 'current' which was indeed a concern I'd highlighted earlier. But might revisit this approach and make them synchronous (with some other locking mechanism) insetad. Also had to make `onChange` async so that I could do some state management, that seems weirder and I don’t like it. Things where testing framework makes things messy with concurrency: - `async let` with things like XCTUnwrap, XCAssertEqual - async operations with XCTAssertThrowsError I hope that once Xcode 16 is released we can instead use Swift Testing. [1] ably/specification#200

Part of #19. References to spec are based on [1] at commit aa7455d. The @preconcurrency imports of ably-cocoa are temporary and will be removed once [2] is done; created #31 for tracking. I’ve decided to, for now, throw ably-cocoa’s ARTErrorInfo for consistency with JS; created #32 to revisit this later. [1] ably/specification#200 [2] ably/ably-cocoa#1962

The @preconcurrency import of ably-cocoa is temporary and will be removed once [2] is done; created #31 for tracking. Part of #19.

@unchecked

Part of #19. References to spec are based on [1] at commit aa7455d. I’ve decided to, for now, throw ably-cocoa’s ARTErrorInfo for consistency with JS; created #32 to revisit this later. We have decided (see [3]) that we’re going to try using actors as our mechanism for concurrency-safe management of mutable state. We accept that this will lead to more of the public API needing to be annotated as `async` (as has happened to Rooms.get here), which in some cases might lead to weird-looking API, and have chosen to accept this compromise in order to get the safety checking offered to us by the compiler, and because of developers’ aversion to writing "@unchecked Sendable". We might not have needed to make this compromise if we had access to Swift 6 / iOS 18’s Mutex type, which allows for synchronous management of mutable state in a way that the compiler is happy with. But, none of the decisions here need to be final; we can see how we feel about the API as it evolves and as our knowledge of the language grows. [1] ably/specification#200 [2] ably/ably-cocoa#1962 [3] #33 (comment)

The @preconcurrency import of ably-cocoa is temporary and will be removed once [2] is done; created #31 for tracking. Part of #19.

@unchecked

Part of #19. References to spec are based on [1] at commit aa7455d. I’ve decided to, for now, throw ably-cocoa’s ARTErrorInfo for consistency with JS; created #32 to revisit this later. We have decided (see [3]) that we’re going to try using actors as our mechanism for concurrency-safe management of mutable state. We accept that this will lead to more of the public API needing to be annotated as `async` (as has happened to Rooms.get here), which in some cases might lead to weird-looking API, and have chosen to accept this compromise in order to get the safety checking offered to us by the compiler, and because of developers’ aversion to writing "@unchecked Sendable". We might not have needed to make this compromise if we had access to Swift 6 / iOS 18’s Mutex type, which allows for synchronous management of mutable state in a way that the compiler is happy with. But, none of the decisions here need to be final; we can see how we feel about the API as it evolves and as our knowledge of the language grows. [1] ably/specification#200 [2] ably/ably-cocoa#1962 [3] #33 (comment)

@unchecked

Part of #19. References to spec are based on [1] at commit aa7455d. I’ve decided to, for now, throw ably-cocoa’s ARTErrorInfo for consistency with JS; created #32 to revisit this later. We have decided (see [3]) that we’re going to try using actors as our mechanism for concurrency-safe management of mutable state. We accept that this will lead to more of the public API needing to be annotated as `async` (as has happened to Rooms.get here), which in some cases might lead to weird-looking API, and have chosen to accept this compromise in order to get the safety checking offered to us by the compiler, and because of developers’ aversion to writing "@unchecked Sendable". We might not have needed to make this compromise if we had access to Swift 6 / iOS 18’s Mutex type, which allows for synchronous management of mutable state in a way that the compiler is happy with. But, none of the decisions here need to be final; we can see how we feel about the API as it evolves and as our knowledge of the language grows. [1] ably/specification#200 [2] ably/ably-cocoa#1962 [3] #33 (comment)

The @preconcurrency import of ably-cocoa is temporary and will be removed once [1] is done; created #31 for tracking. Part of #19. [1] ably/ably-cocoa#1962

@unchecked

Part of #19. References to spec are based on [1] at commit aa7455d. I’ve decided to, for now, throw ably-cocoa’s ARTErrorInfo for consistency with JS; created #32 to revisit this later. We have decided (see [2]) that we’re going to try using actors as our mechanism for concurrency-safe management of mutable state. We accept that this will lead to more of the public API needing to be annotated as `async` (as has happened to Rooms.get here), which in some cases might lead to weird-looking API, and have chosen to accept this compromise in order to get the safety checking offered to us by the compiler, and because of developers’ aversion to writing "@unchecked Sendable". We might not have needed to make this compromise if we had access to Swift 6 / iOS 18’s Mutex type, which allows for synchronous management of mutable state in a way that the compiler is happy with. But, none of the decisions here need to be final; we can see how we feel about the API as it evolves and as our knowledge of the language grows. [1] ably/specification#200 [2] #33 (comment)

Part of #19.

@unchecked

Part of #19. References to spec are based on [1] at commit aa7455d. I’ve decided to, for now, throw ably-cocoa’s ARTErrorInfo for consistency with JS; created #32 to revisit this later. We have decided (see [2]) that we’re going to try using actors as our mechanism for concurrency-safe management of mutable state. We accept that this will lead to more of the public API needing to be annotated as `async` (as has happened to Rooms.get here), which in some cases might lead to weird-looking API, and have chosen to accept this compromise in order to get the safety checking offered to us by the compiler, and because of developers’ aversion to writing "@unchecked Sendable". We might not have needed to make this compromise if we had access to Swift 6 / iOS 18’s Mutex type, which allows for synchronous management of mutable state in a way that the compiler is happy with. But, none of the decisions here need to be final; we can see how we feel about the API as it evolves and as our knowledge of the language grows. [1] ably/specification#200 [2] #33 (comment)

Based on the simplified requirements described in #19. This doesn’t include the emission of a room status change; will do that in a separate PR.

Based on the simplified requirements described in #19. The decision from 7d6acde to use actors as the mechanism for managing mutable state means that I’ve had to make RoomStatus.{ current, error, onChange(bufferingPolicy:) } async. As mentioned there, if later on we decide this is too weird an API, then we can think of alternatives. I really wanted to avoid making DefaultRoomStatus an actor; I tried to find a way to isolate this state to the DefaultRoom actor (who logically owns this state), by trying to make the DefaultRoomStatus access the DefaultRoom-managed state, but I was not successful and didn’t want to sink much time into it. It means that DefaultRoomStatus has suspension points in order to access its current state, which I am not happy about. But we can revisit later, perhaps armed with more knowledge of concurrency and in less of a rush to get some initial functionality implemented. Resolves #19.

Based on the simplified requirements described in #19. The decision from 7d6acde to use actors as the mechanism for managing mutable state means that I’ve had to make RoomStatus.{ current, error, onChange(bufferingPolicy:) } async. As mentioned there, if later on we decide this is too weird an API, then we can think of alternatives. I really wanted to avoid making DefaultRoomStatus an actor; I tried to find a way to isolate this state to the DefaultRoom actor (who logically owns this state), by trying to make the DefaultRoomStatus access the DefaultRoom-managed state, but I was not successful and didn’t want to sink much time into it. It means that DefaultRoom has suspension points in order to access its current state, which I am not happy about. But we can revisit later, perhaps armed with more knowledge of concurrency and in less of a rush to get some initial functionality implemented. Resolves #19.

This is based on [1] at aa7455d. It’s generated some questions, which I’ve asked on that PR. I started implementing this as part of #19, before realising that implementing the spec is not the aim of that task. So, putting this work on hold until we pick it up again in #28. So far, only the ATTACH operation is implemented. [1] ably/specification#200

This is based on [1] at aa7455d. It’s generated some questions, which I’ve asked on that PR. I started implementing this as part of #19, before realising that implementing the spec is not the aim of that task. So, putting this work on hold until we pick it up again in #28. So far, only the ATTACH operation is implemented. [1] ably/specification#200 I have since updated with error names @ bcb7390

This is based on [1] at aa7455d. It’s generated some questions, which I’ve asked on that PR. I started implementing this as part of #19, before realising that implementing the spec is not the aim of that task. So, putting this work on hold until we pick it up again in #28. So far, only the ATTACH operation is implemented. [1] ably/specification#200 I have since updated with error names @ bcb7390 other stuff done since first version of this commit: - start implementing the low-hanging fruit of DETACH

This is based on [1] at aa7455d. It’s generated some questions, which I’ve asked on that PR. I started implementing this as part of #19, before realising that implementing the spec is not the aim of that task. So, putting this work on hold until we pick it up again in #28. So far, only the ATTACH operation is implemented. [1] ably/specification#200 I have since updated with error names @ bcb7390 other stuff done since first version of this commit: - start implementing the low-hanging fruit of DETACH all the infrastructure in place for feature-specific errors used generic type for contributor so that i can access the mock-specific properties of the contributor's channel in the tests without having to create channels and then create contributors from those wip detachment errors further with detachment errors wip trying to see if there aren't 2 status updates Revert "wip trying to see if there aren't 2 status updates" This reverts commit 63dd9b9. Doesn’t work. implement CHA-RL2h2 wip channel detach retry a few updates to older bits implement more of RELEASE comment wip done RELEASE

This is based on [1] at aa7455d. It’s generated some questions, which I’ve asked on that PR. I started implementing this as part of #19, before realising that implementing the spec is not the aim of that task. So, putting this work on hold until we pick it up again in #28. So far, only the ATTACH operation is implemented. [1] ably/specification#200 I have since updated with error names @ bcb7390 other stuff done since first version of this commit: - start implementing the low-hanging fruit of DETACH all the infrastructure in place for feature-specific errors used generic type for contributor so that i can access the mock-specific properties of the contributor's channel in the tests without having to create channels and then create contributors from those wip detachment errors further with detachment errors wip trying to see if there aren't 2 status updates Revert "wip trying to see if there aren't 2 status updates" This reverts commit 63dd9b9. Doesn’t work. implement CHA-RL2h2 wip channel detach retry a few updates to older bits implement more of RELEASE comment wip done RELEASE OK, I believe that this is now based on b4a495e of the spec some further comments spec points documentation based on JS rules [1] @ 8c9ce8b, but decided that: - if a test doesn't relate to a spec point, it doesn't need any markers - there should be a way to know that a spec point is tested across multiple tests [1] https://github.com/ably/ably-js/blob/main/CONTRIBUTING.md#tests-alignment-with-the-ably-features-specification

lawrence-forooghian added the enhancement New feature or improved functionality. label Aug 19, 2024

lawrence-forooghian self-assigned this Aug 19, 2024

lawrence-forooghian added a commit that referenced this issue Aug 19, 2024

wip #19

b82ccbe

equatable conformances are for tests, but not sure if right thing to do

lawrence-forooghian mentioned this issue Aug 28, 2024

Chat Room: start implementing Andy’s room lifecycle spec #28

Closed

lawrence-forooghian mentioned this issue Aug 28, 2024

[WIP, ECO-4952] Implement room lifecycle spec #30

Closed

lawrence-forooghian mentioned this issue Aug 29, 2024

[ECO-4936] Implement the ability to fetch a room #33

Merged

lawrence-forooghian added a commit that referenced this issue Aug 29, 2024

Implement DefaultChatClient.rooms

3c35f4f

The @preconcurrency import of ably-cocoa is temporary and will be removed once [2] is done; created #31 for tracking. Part of #19.

lawrence-forooghian added a commit that referenced this issue Aug 29, 2024

Implement DefaultChatClient.rooms

202161b

The @preconcurrency import of ably-cocoa is temporary and will be removed once [2] is done; created #31 for tracking. Part of #19.

lawrence-forooghian added a commit that referenced this issue Aug 29, 2024

Implement DefaultChatClient.rooms

c50af5d

The @preconcurrency import of ably-cocoa is temporary and will be removed once [1] is done; created #31 for tracking. Part of #19. [1] ably/ably-cocoa#1962

lawrence-forooghian added a commit that referenced this issue Aug 29, 2024

Implement DefaultChatClient.rooms

eb12e07

Part of #19.

lawrence-forooghian added a commit that referenced this issue Sep 2, 2024

Add the ability to attach and detach a room

c272c82

Based on the simplified requirements described in #19. This doesn’t include the emission of a room status change; will do that in a separate PR.

lawrence-forooghian added a commit that referenced this issue Sep 2, 2024

Implement the ability to attach and detach a room

f284387

Based on the simplified requirements described in #19. This doesn’t include the emission of a room status change; will do that in a separate PR.

lawrence-forooghian mentioned this issue Sep 2, 2024

[ECO-4936] Implement the ability to attach and detach a room #35

Merged

lawrence-forooghian added a commit that referenced this issue Sep 2, 2024

Implement the ability to attach and detach a room

4198c7f

Based on the simplified requirements described in #19. This doesn’t include the emission of a room status change; will do that in a separate PR.

lawrence-forooghian added a commit that referenced this issue Sep 2, 2024

Implement the ability to attach and detach a room

21da342

Based on the simplified requirements described in #19. This doesn’t include the emission of a room status change; will do that in a separate PR.

lawrence-forooghian added a commit that referenced this issue Sep 3, 2024

Implement the ability to attach and detach a room

09e54f6

Based on the simplified requirements described in #19. This doesn’t include the emission of a room status change; will do that in a separate PR.

lawrence-forooghian mentioned this issue Sep 3, 2024

Find a way to clean up arrays of subscriptions #36

Open

lawrence-forooghian mentioned this issue Sep 3, 2024

Implement room status change upon attach or detach #37

Merged

lawrence-forooghian mentioned this issue Sep 3, 2024

Implement logging #38

Closed

lawrence-forooghian closed this as completed in #37 Sep 5, 2024

lawrence-forooghian closed this as completed in a63b22b Sep 5, 2024

This was referenced Sep 30, 2024

[ECO-4936] Implement the ability to attach and detach a room sourcery-ai-experiments/ably-chat-swift#16

Closed

[ECO-4936] Implement the ability to attach and detach a room sourcery-ai-experiments/ably-chat-swift#18

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Chat room skeleton #19

Chat room skeleton #19

lawrence-forooghian commented Aug 19, 2024 •

edited by sync-by-unito bot

Loading

lawrence-forooghian commented Aug 28, 2024

lawrence-forooghian commented Aug 28, 2024

Chat room skeleton #19

Chat room skeleton #19

Comments

lawrence-forooghian commented Aug 19, 2024 • edited by sync-by-unito bot Loading

lawrence-forooghian commented Aug 28, 2024

lawrence-forooghian commented Aug 28, 2024

lawrence-forooghian commented Aug 19, 2024 •

edited by sync-by-unito bot

Loading