-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ECO-5135] Updates the Chat docs to include code examples for chat-swift #2315
base: main
Are you sure you want to change the base?
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (f7ca60d)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's quite a few places where we can combine, reduce and tidy up the amount of text required by using spans instead of blangs. I haven't commented on them all, after identifying the pattern. We also seem to have rewritten all text to not mention listeners in Swift - is this deliberate?
content/chat/connect.textile
Outdated
@@ -63,12 +71,17 @@ blang[react]. | |||
|
|||
blang[javascript]. | |||
|
|||
blang[swift]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
blang[swift]. | |
blang[javascript,swift]. |
This can roll up into the JS one above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This overlaps with https://github.com/ably/docs/pull/2300/files
content/chat/connect.textile
Outdated
@@ -24,6 +25,9 @@ A connection can have any of the following statuses: | |||
blang[javascript]. | |||
Use the "@current@":https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat_js.ConnectionStatus.html#current property to check which status a connection is currently in: | |||
|
|||
blang[swift]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's only the link that will be different then I think it may be clearer to combine it into a single block, e.g.:
blang[javascript,swift].
Use the <span lang="javascript>"@current@":https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat_js.ConnectionStatus.html#current</span><span lang="swift"></span> property to check which status a connection is currently in:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, here the property name is also different (which is probably something that should be fixed fyi @AndyTWF @lawrence-forooghian @ttypic)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I spoke to Andy about this separately and it has been updated for JS in a separate PR (#2300).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That PR modifies the same lines, I guess it should be merged first before me to proceed with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
content/chat/connect.textile
Outdated
blang[javascript]. | ||
Use the "@connection.status.onChange()@":https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat_js.ConnectionStatus.html#onChange method to register a listener for status change updates: | ||
|
||
blang[react]. | ||
Listeners can also be registered to monitor the changes in connection status. Any hooks that take an optional listener to monitor their events, such as typing indicator events in the @useTyping@ hook, can also register a status change listener. Changing the value provided for a listener will cause the previously registered listener instance to stop receiving events. All messages will be received by exactly one listener. | ||
|
||
blang[swift]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could also roll into the JS line above with spans for the link change.
However, if bufferingPolicy
is required then instead I think this should be kept separate, but we should explicitly state it needs to be set (can this not be a default config?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's also connection.status.onChange
vs connection.onStatusChange
which I think also should be fixed in the SDK (not sure which one, looks like the status.onChange
is more idiomatic).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
connection.onStatusChange
was the one agreed in the DR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is also addressed here, right? Why wasn't it merged yet? @AndyTWF @m-hulbert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not been merged yet because the we haven't yet released the chat-js SDK version containing the change. It's due to be merged next week
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
created an issue re defaults ably/ably-chat-swift#172 @m-hulbert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
content/chat/connect.textile
Outdated
@@ -101,6 +121,13 @@ blang[javascript]. | |||
|
|||
blang[react]. | |||
|
|||
blang[swift]. | |||
To stop the subscription from firing new status events, call its @finish()@ function: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can use the same terminology as JS in terms of removing the listener. We can put them in the same block as the JS text and code with span tags then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's also something that can be narrowed down to using only one term in all SDKs. Now js uses off
, kotlin unsubscribe
and swift finish
. WDYT @ttypic @lawrence-forooghian
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
content/chat/connect.textile
Outdated
@@ -112,6 +139,9 @@ blang[javascript]. | |||
|
|||
blang[react]. | |||
Any hooks that take an optional listener to monitor their events, such as typing indicator events in the @useTyping@ hook, can also register a listener to be notified of, and handle, periods of discontinuity. | |||
|
|||
blang[swift]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this materially change from the JS? Again, I think we could put this in the JS block with span tags on the method name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And the method name itself can be the same @umair-ably @lawrence-forooghian
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For ref, it's onDiscontinuity
in JS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Created an issue ably/ably-chat-swift#173
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
content/chat/rooms/index.textile
Outdated
|
||
h3(#release). Release a room | ||
|
||
Releasing a room allows the underlying resources to be garbage collected or released. | ||
|
||
Releasing a room may be optional for many applications. If you have multiple transient rooms, such as in the case of a 1:1 support chat, then it is may be more beneficial. | ||
Once "@rooms.release()@":https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat_js.Rooms.html#release has been called, the room will be unusable and a new instance will need to be created using "@rooms.get()@":#create if you want to reuse it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this just needed swift
added to the blang, as it isn't relevant to React.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React is so different in every aspect, doesn't it make sense to have a different .textile file for it? @m-hulbert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, we've had a few conversations around trying that. We did it for Pub/Sub but the info gets a bit lost when you use them.
content/chat/rooms/index.textile
Outdated
@@ -180,6 +209,9 @@ blang[javascript]. | |||
blang[react]. | |||
Use the @roomStatus@ property to view the current "@Room@":https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat_js.Room.html status changes. The @roomError@ property is its associated error. Any hooks that take an optional listener have these properties available in their response, such as @useMessages@ or @useTyping@. It is more common that you will monitor the room status in the specific feature hooks rather than needing to use @useRoom@. These events are related to the room instance of the nearest "@ChatRoomProvider@":https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/functions/chat_react.ChatRoomProvider.html. For example, with the @useMessages@ hook: | |||
|
|||
blang[swift]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as for connection - we can merge this into the JS one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This overlaps with https://github.com/ably/docs/pull/2300/files
content/chat/rooms/index.textile
Outdated
blang[swift]. | ||
You can also create a subscribtion to the room status updates. An event will be emitted whenever the status of the room changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be separate to the JS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we agree to mitigate the difference between "registering a listener" and "creating a subscribtion" then sure it can be merged together.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you recommend here @m-hulbert? Broadly, Swift and JS are doing the same thing - though in JS we tend to prefer the term listener, rather than subscription. Are the concepts close enough that we can just use the same term across the board?
I'd personally prefer we don't use different terms across the ecosystems, so its not confusing for users using both
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the majority of SDKs we refer to listeners still in Pub/Sub, hence me questioning the difference here. It doesn't appear unidiomatic to Swift to continue with that, and that would be my recommendation. But I wasn't sure if there was a reason we'd actively avoided it in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what would be the final decision for this PR? @m-hulbert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
content/chat/setup.textile
Outdated
|
||
The SDK is distributed as a Swift Package and can be installed using Xcode or by adding it as a dependency in your package’s @Package.swift@. | ||
|
||
Please note the version listed in the language dropdown selector on this page corresponds to the Ably Pub/Sub SDK version, and not the Chat SDK version. Information on the latest Chat SDK release can be found at "https://github.com/ably/ably-chat-swift":https://github.com/ably/ably-chat-swift. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be fixed within the next few weeks, so can be removed. It's not something we state in any other product or language currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
content/chat/setup.textile
Outdated
|
||
To install the @ably-chat-swift@ package in your Xcode Project: | ||
* Paste @https://github.com/ably/ably-chat-swift@ in the Swift Packages search box. ( Xcode project → Swift Packages.. . → @+@ button) | ||
* Select the AblyChat SDK for your target. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* Select the AblyChat SDK for your target. | |
* Select the Ably Chat SDK for your target. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
content/chat/rooms/reactions.textile
Outdated
|
||
blang[react]. | ||
Subscribe to room reactions with the "@useRoomReactions@":https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/functions/chat_react.useRoomReactions.html hook. Supply an optional listener to receive the room reactions. | ||
|
||
blang[swift]. | ||
Subscribe to a room reactions by creating a subscription. Use the @occupancy.subscribe(bufferingPolicy: .unbounded)@ method in a room to receive reactions: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be reactions
here instead of occupancy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
content/chat/connect.textile
Outdated
@@ -24,6 +25,9 @@ A connection can have any of the following statuses: | |||
blang[javascript]. | |||
Use the "@current@":https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat_js.ConnectionStatus.html#current property to check which status a connection is currently in: | |||
|
|||
blang[swift]. | |||
Use the "@status@":https://sdk.ably.com/builds/ably/#status property to check which status a connection is currently in: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears to be a dead link right now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm going to insert links that will look like:
"@status()@":https://sdk.ably.com/builds/ably/ably-chat-swift/main/typedoc/interfaces/chat.Rooms.html#status
Which are also yet dead, but with a potential to go live as soon as swift docs generation PR merged (ably/ably-chat-swift#165)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
content/chat/connect.textile
Outdated
blang[javascript]. | ||
Use the "@connection.status.onChange()@":https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat_js.ConnectionStatus.html#onChange method to register a listener for status change updates: | ||
|
||
blang[react]. | ||
Listeners can also be registered to monitor the changes in connection status. Any hooks that take an optional listener to monitor their events, such as typing indicator events in the @useTyping@ hook, can also register a status change listener. Changing the value provided for a listener will cause the previously registered listener instance to stop receiving events. All messages will be received by exactly one listener. | ||
|
||
blang[swift]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
connection.onStatusChange
was the one agreed in the DR
content/chat/connect.textile
Outdated
@@ -112,6 +139,9 @@ blang[javascript]. | |||
|
|||
blang[react]. | |||
Any hooks that take an optional listener to monitor their events, such as typing indicator events in the @useTyping@ hook, can also register a listener to be notified of, and handle, periods of discontinuity. | |||
|
|||
blang[swift]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For ref, it's onDiscontinuity
in JS
blang[swift]. | ||
|_. Parameter |_. Description | | ||
| start | Earliest time to retrieve messages from, as a unix timestamp in milliseconds. Messages with a timestamp equal to, or greater than, this value will be returned. | | ||
| end | Latest time to retrieve messages from, as a unix timestamp in milliseconds. Messages with a timestamp less than this value will be returned. | | ||
| orderBy | The direction in which to retrieve messages from; either @oldestFirst@ or @newestFirst@. | | ||
| limit | Maximum number of messages to be retrieved, up to 1,000. | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This divergence hasn't yet had a DR and thus hasn't been incorporated into JS - it was sorta an informal Slack conversation.
I suggest we allow the lack of parity for now until we get a chance to update JS - shouldn't take long but I don't want to block this PR getting merged.
content/chat/rooms/index.textile
Outdated
@@ -62,8 +67,11 @@ blang[react]. | |||
|
|||
blang[javascript]. | |||
|
|||
blang[swift]. | |||
|
|||
When you create or retrieve a room using @rooms.get()@, you need to choose which additional chat features you want enabled for that room. This is configured by passing a "@RoomOptions@":https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat_js.RoomOptions.html object as the second argument. In addition to setting which features are enabled, @RoomOptions@ also configures the properties of the associated features, such as the timeout period for typing indicators. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be referencing chat-js's type descriptions in Swift, though - as opposed to Swift's own?
content/chat/rooms/index.textile
Outdated
blang[swift]. | ||
You can also create a subscribtion to the room status updates. An event will be emitted whenever the status of the room changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you recommend here @m-hulbert? Broadly, Swift and JS are doing the same thing - though in JS we tend to prefer the term listener, rather than subscription. Are the concepts close enough that we can just use the same term across the board?
I'd personally prefer we don't use different terms across the ecosystems, so its not confusing for users using both
content/chat/rooms/messages.textile
Outdated
@@ -97,6 +108,13 @@ blang[javascript]. | |||
blang[react]. | |||
When you unmount the component that is using the @useMessages@ hook, it will automatically handle unsubscribing any associated listeners registered to receive messages. | |||
|
|||
blang[swift]. | |||
To cancel the room messages subscribtion, call the provided @finish()@ method: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General question, is finish
idiomatic in Swift? In JS we preferred unsubscribe
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
let isPresent = try await room.presence.isUserPresent(clientID: "clemons123") | ||
``` | ||
|
||
blang[react]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra blangs here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated
@@ -84,29 +92,42 @@ blang[javascript]. | |||
```[javascript] | |||
const room = chatClient.rooms.get('basketball-stream', {typing: {timeoutMs: 5000}}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't it duplicates L80 sample? @m-hulbert
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One shows using the default values that are configured, and the other setting custom values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Default is line 91, custom are lines 76 and 97, both with {timeoutMs: 5000}
. Am I missing something?
a408a0e
to
a78b89b
Compare
content/chat/rooms/messages.textile
Outdated
Unsubscribe from messages to remove previously registered listeners. | ||
|
||
Use the "@unsubscribe()@":https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat-js.MessageSubscriptionResponse.html#unsubscribe function returned in the @subscribe()@ response to remove a listener: | ||
Use the <span lang="javascript">"@unsubscribe()@":https://sdk.ably.com/builds/ably/ably-chat-js/main/typedoc/interfaces/chat-js.MessageSubscriptionResponse.html#unsubscribe</span><span lang="swift">"@unsubscribe()@":https://sdk.ably.com/builds/ably/ably-chat-swift/main/typedoc/interfaces/chat.MessageSubscription.html#unsubscribe</span> function returned in the @subscribe()@ response to remove a listener: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My Swift isn't great, but I don't think the subscription returns an unsubscribe function like in JS, and it's an independent method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understood we decided to go with the same descriptions for listeners. Or was it only for creation of the subscription/listener?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes sense for the creation of the subscription for sure. But correct me if I'm wrong, but this doesn't make sense for Swift.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@m-hulbert done fa775da
Description
Updates the Chat docs to include code examples for chat-swift
https://ably.atlassian.net/browse/ECO-5135
Review
/chat/setup?lang=swift
/chat/connect?lang=swift
/chat/rooms?lang=swift
/chat/rooms/messages?lang=swift
/chat/rooms/presence?lang=swift
/chat/rooms/occupancy?lang=swift
/chat/rooms/typing?lang=swift
/chat/rooms/reactions?lang=swift
/chat/rooms/history?lang=swift