Skip to content
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

Add opt-out of implicit attach when subscribing #206

Merged
merged 1 commit into from
Sep 10, 2024

Conversation

lawrence-forooghian
Copy link
Collaborator

@lawrence-forooghian lawrence-forooghian commented Sep 6, 2024

Add an attachOnSubscribe channel option, which, if set to false, allows users to opt out of the implicit attach that’s triggered by subscribe-ing to channel or presence messages.

The Chat SDK has decided that in its API, subscribing to a room’s messages or presence messages should not trigger a channel attach operation (see decision record). The JS Chat SDK uses private ably-js API to, essentially, call subscribe without triggering an implicit attach. For the Swift and Kotlin SDKs, we’d like to avoid this, hence this new API.

I’m not sure what’s the better approach out of adding a new channel option (I don’t love the fact that we just ignore the user-provided “on attach” callback) or adding a new method called something like subscribeWithoutAttach (which, given that it’d be part of the public API, might be quite confusing for users when it appears in their IDE autocomplete or whatever).

Resolves #202.

Copy link
Member

@SimonWoolf SimonWoolf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fine by me

Copy link
Contributor

@AndyTWF AndyTWF left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

We should consider whether we want this to be default behaviour in future major SDK versions.

Add an attachOnSubscribe channel option, which, if set to false, allows users
to opt out of the implicit attach that’s triggered by `subscribe`-ing to
channel or presence messages.

The Chat SDK has decided that in its API, subscribing to a room’s messages or
presence messages should not trigger a channel attach operation (see [1] for
decision record). The JS Chat SDK uses private ably-js API to, essentially,
call `subscribe` without triggering an implicit attach. For the Swift and
Kotlin SDKs, we’d like to avoid this, hence this new API.

I’m not sure what’s the better approach out of adding a new channel option (I
don’t love the fact that we just ignore the user-provided “on attach” callback)
or adding a new method called something like `subscribeWithoutAttach` (which,
given that it’d be part of the public API, might be quite confusing for users
when it appears in their IDE autocomplete or whatever).

Resolves #202.

[1] https://ably.atlassian.net/wiki/spaces/CHA/pages/3156705308/CHADR-038+API+Design+Separating+Listeners+from+Lifecycle
@sacOO7
Copy link
Collaborator

sacOO7 commented Sep 10, 2024

Okay noob question, why don't we just create plain channel when subscribe is called.

void subscribe("room1") Channel {
   return new realtimeClient.Channels.Get("room1")
}

This shouldn't send attach message and just create a room instead

@lawrence-forooghian
Copy link
Collaborator Author

lawrence-forooghian commented Sep 10, 2024

Okay noob question, why don't we just create plain channel when subscribe is called.

The aim of subscribe is to register a listener that will be called when the channel receives a message. Your example fetches a channel but doesn't register a listener.

@SimonWoolf
Copy link
Member

We should consider whether we want this to be default behaviour in future major SDK versions.

I would endorse this, never liked implicit attach. (especially never liked the asymmetry of 'implicit attach on first subscribe but no implicit detach on last unsubscribe')

@AndyTWF
Copy link
Contributor

AndyTWF commented Sep 10, 2024

We should consider whether we want this to be default behaviour in future major SDK versions.

I would endorse this, never liked implicit attach. (especially never liked the asymmetry of 'implicit attach on first subscribe but no implicit detach on last unsubscribe')

In Chat we took the approach of having subscribe having no bearing at all on the chat room lifecycle (in all cases). Any listener registration is purely synchronous. Similar for any other operation that would have a side effect (e.g. presence get), we either have it behind a promise that won't resolve until attach is initiated explicitly, or it simply fails and informs the user that they need to attach first.

Copy link
Contributor

@ttypic ttypic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this approach

@maratal
Copy link
Collaborator

maratal commented Sep 10, 2024

we just ignore the user-provided “on attach” callback

Maybe do attach when user provides this callback and do not do by default if not?

@lawrence-forooghian
Copy link
Collaborator Author

Maybe do attach when user provides this callback and do not do by default if not?

That'd be a breaking API change — the current expectation is that if you call subscribe then it attaches.

Copy link
Collaborator

@maratal maratal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lawrence-forooghian lawrence-forooghian merged commit 44851b5 into main Sep 10, 2024
2 checks passed
@lawrence-forooghian lawrence-forooghian deleted the 202-subscribe-without-attach branch September 10, 2024 21:28
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Add a way to subscribe to realtime channel and presence messages without triggering an attach
6 participants