-
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
Review API for exposing channel attach flags #279
Comments
@SimonWoolf @paddybyers FYI I've tagged this for v1.1 |
This is a prerequisite for #374 and a couple other proposed channel flags, we should make a decision on this so client libs can start implementing for 1.1. two most obvious options:
any opinions, or third options? In either case, we need to decide:
|
I like this
we can even simplify it as
Yes, all the way. But I am not sure what's the impact of this on the backend.
I think we should come up with default values. So this:
under the hood becomes
and this
becomes
|
For js we could, but for strongly typed languages it's a bit of a hassle for it to take either a channelOptions or an array, so probably better stick with just a channelOptions for simplicity.
I don't follow what you're proposing. The default values of the flags that are modes are all on, so To re-iterate: the problem here is whether passing any modes at all means you need to pass a complete set of modes, given that we want the modal flags to default to on but be arbitrarily specifiable if needed, but the non-modal flags to default to off. I think my preferred solution is to separate them in the api -- so you have a channelOptions that has a |
Why not having them all belonging to the same realtime.channels.get(channelName, { publish: true, subscribe: true , noBacklog: true, ... }); Feels more explicit to me. |
I think the proposal to modify So my proposal would be channel options for Additionally, we've discussed this before in detail and agreed this was the correct way to approach it. An attach now currently changes the state of a channel, but only so much as it's a temporary state change whilst it waits for an If we are considering flags though, I think we should think a little broader if we are making an API change now. For example:
So before we can think about the API in clients, I think we also need to think carefully about the protocol itself. At a protocol level, I would argue that:
Considerations of this:
Now moving onto the API, I think we should approach this as:
So to summarise:
|
A few comments/questions:
|
I strongly disagree. We didn't implement
Because if the client lib now implicitly requests a state it does not have access to, the channel will become failed. Case in point, an
We don't do this with capabilities.
Sure, but that is a server responsibility and not an implementation detail the client needs to worry about.
I thought that too at first, but then realised that's unnecessary. If you request ATTACH with none, then ATTACH with subscribe, then ATTACH with subscribe + publish, before any ATTACHED is returned, who cares? You still end up with the same result. If any ATTACH request was |
nb also need to decide https://github.com/ably/realtime/pull/1866/files#diff-dbf0135e92ab64a9e4756f3e52b9b5ceR77 |
@SimonWoolf how much of this is now done with the new channel params? Just wondering if we can close this long issue perhaps and open another with only what is outstanding. |
Sure, I think we can say this is now basically all fixed by #766 👍 |
ably/ably-js#377 adds an undocumented API (a secret additional argument to attach()) to specify channel attach flags, which is used by the realtime
channel_attach_flags
tests. Per ably/ably-js@1cfe662#commitcomment-21065665 , we should review this to discuss how we want to expose this officially (and whether we want to at all).The ably-js temporary api currently takes an array of strings specified in the ProtocolMessage
flags
object, e.g.channel.attach(['PUBLISH','SUBSCRIBE'], (err) => console.log(err))
.The text was updated successfully, but these errors were encountered: