v1.2 API design

[tcard]

This issue tracks and discuss the changes to be made to the public API in order to make it 1.2 spec-compliant.

Subscriptions: from channels to callbacks

For some features, we make a long-lived subscription to a series of events that the caller receives. The spec says those events are delivered by calling a callback. In Go, instead, we were sending them to channels, from which user code receive events at their own. For 1.2 we're taking callbacks. See the relevant discussion.

So we go from something like:

ch := make(chan ably.ChannelStateChange)
channel.On(ably.ChannelStateChange, ably.ConnectionEventAttached)
for change := range ch {
    // ...
}

To:

channel.On(ably.ConnectionEventAttached, func(change ably.ChannelStateChange) {
    // ...
})

The list of such features is:

• Connection events (v1.2 event emitter for connection (RTN4) #144)
• Channel events (RTL2: EventEmitter for channel events; remove old State #190)
• Messages (v1.2-compliant message and presence publish and subscribe #202)
• Pressence messages (v1.2-compliant message and presence publish and subscribe #202)

Options

We've decided to implement the classes from the spec that define options as functional options instead.

Each option is defined as a package-level function, with a With prefix. Additionally, except ClientOptions, each function has first the name of what they're options for as prefix (e. g. RestChannel.publish(params?) → PublishBatchWithParams). (Discussion below.)

• ClientOptions and AuthOptions (v1.2 ClientOptions #145)
  • Converted to package-level functions (Implement options as package-level functions #205).
• ChannelOptions (v1.2 ChannelOptions #146)
  • Converted to package-level functions (Implement options as package-level functions #205).

Optional arguments

Optional arguments are translated as functional options, as described above. (Discussion below.)

If an optional argument is itself an options aggregate per the spec, the options "collapse" into a single level.

io actions: from Result to blocking calls

The spec marks with a io annotation values that result from making an IO operation (to the network, typically). In some environments (JavaScript, Cocoa) those are implemented by passing them to a callback or an equivalent Promise-style mechanism. In others, it's just a normal function call that blocks until the operation is complete and returns the value. If the user wants to do more than one thing at once, they use threads or some equivalent.

In Go, it's common-place to do the latter: calls that hit the network block for the whole duration, and if you want asynchrony, you spawn goroutines. But we're using a two-stage Promise-like thing in which a method first initiates the operation on the background and returns (ably.Result, error) immediately; then, you can call ably.Result.Wait() to wait for the operation to complete, which may produce.

This is weird for Go (see ably/docs#155) and we're switching to fully blocking calls for the duration of the operation.

So this:

result, err := channel.Attach()
if err != nil {
    // ...
}
err = result.Wait()
if err != nil {
    // ...
}

Becomes:

err := channel.Attach()
if err != nil {
    // ...
}

Context

In Go, it has become common-place for IO operations to take a context.Context. For 1.2, we will implement the io annotation by adding a first context.Context argument in such methods.

For HTTP requests, the context is passed in the underlying *http.Request.

For operations that result in WebSocket sends and receives, the situation is a bit more complex. A WebSocket is ultimately a net.Conn, which is just a io.Reader and io.Writer; those don't take contexts. So the context would only be used to unblock the call at the point it's waiting for a WebSocket receive, e. g. an ATTACHED message.

This raises the question: do we then act as if the operation has failed entirely? E. g. do we go back to the INITIALIZED/DETACHED state from ATTACHING?

1. If we do, then the client and the server may disagree on the channel state. Normally, this possible discrepancy is resolved because the connection is broken and reestablished after a receive timeout or some other network condition that causes the error.
2. If we don't, then we cause the surprising effect that the operation returns an error, yet we see the consequences of success anyway, e. g. start receiving messages on the channel.
3. Another possibility is not taking contexts at all for WebSocket operations, but that seems inconsistent.

I'd say we can go with 2 and just implement a transparent way of dealing with the inconsistency, e. g. automatically detach once the ATTACHED arrives. This should be in the spec too.

Misc: names, argument order, etc.

The rest would just be straightforward: make sure we expose only the API from the spec, in the form and shape in which the spec defines it. This includes renaming, removing things or moving them to separate packages, reordering arguments, etc.

┆Issue is synchronized with this Jira Story by Unito

[lmars]

@tcard this all sounds good to me 👍

On the question around contexts, I discussed with @paddybyers and we concluded that the context should just be used as a timeout to unblock the goroutine but have no further side-effects.

In particularly, if ctx in channel.Attach(ctx) gets cancelled then the call returns an error but the library will still continue as normal if the channel ends up getting attached, as this matches the behaviour of other client libraries that have timeouts.

[tcard]

I've noticed that the spec has been liberally adding optional parameters in minor revisions (see ably/specification#41).

If that's the case, then we need a consistent strategy to be able to incorporate new optional parameters as non-breaking changes.

Unfortunately, I see no ideal option here:

1. Making the API a mess by preventively adding some kind of "bag of optional parameters" to all methods.
2. Relax what we consider a breaking change. Introduce that "bag of optional parameters" only for methods that have at least one such optional parameter (not already translated as an additional method or non-optional parameter). The "bag" would then be the variadic argument. The method's signature would change but the actual calls wouldn't, so even though strictly speaking we do introduce breaking changes, almost no usages will be affected.
3. Do as 2, but by introducing new methods that take such "bag". This prevents all breaking changes, but pollutes the library.
4. Just assume that we will continue to introduce breaking changes in minor revisions in the future.

3 seems to me the most balanced option.

The "bag" should follow the functional options pattern described above.

Getting some assurance from the spec that some methods won't ever add new optional parameters in minor versions. Then we can stop being so defensive for those methods. For the rest, we can go with 1-3.

[lmars]

I think 3 is the way to go.

So to clarify, I'm assuming you mean implement each method just with the required parameters, and complement those with a method suffixed with WithOptions that takes the required parameters plus a variadic list of functional options for those methods that have optional parameters?

For example, assuming the following in the spec:

class Foo:
    bar(baz: string, qux?: string)

That would be implemented as:

func (Foo) Bar(baz string)

type BarOptions func(Foo)

func WithQux(qux string) BarOptions

func (Foo) BarWithOptions(baz string, opts ...BarOptions)

which would be used like:

foo.Bar(baz)

foo.BarWithOptions(baz, WithQux(qux))

I think this is a good approach as it means we don't have to add support for options to every method, but can support options for those methods in the future by introducing the WithOptions version of it.

[lmars]

@tcard some things I've noticed from looking through the current API as it stands on the feature/v1.2-messages branch (which I assume is the most up-to-date):

• there is a ChannelOptions type but the methods that I assume it relates to (i.e. Channels.Get and RestChannels.Get) take ...ChannelOption, this seems incorrect?

• given we've renamed RestChannel to RESTChannel, should we also rename RestChannels to RESTChannels and RestPresence to RESTPresence (although the spec does reference RestChannel, so perhaps this is going off-spec)?

• given we're adopting functional options, I think constructors like NewRealtime and NewREST should take a variadic list of functional options rather than a mandatory struct (i.e. a default client is ably.NewRealtime() rather than ably.NewRealtime(ably.ClientOptions{}), or one with a key is ably.NewRealtime(ably.WithKey("xxx.xxx")))

• should the following take a context.Context?

  • Realtime.Connect
  • RealtimeChannel.History
  • RealtimePresence.Enter (and remove the Result return value)
  • RealtimePresence.EnterClient (and remove the Result return value)
  • RealtimePresence.Leave (and remove the Result return value)
  • RealtimePresence.LeaveClient (and remove the Result return value)
  • RealtimePresence.Update (and remove the Result return value)
  • RealtimePresence.UpdateClient (and remove the Result return value)
  • RealtimePresence.Get
  • Connection.Connect
  • Connection.Ping

• should REST.Request and Realtime.Request have WithOptions variants as per the discussion above?

[lmars]

@tcard it would be good to discuss and agree the approach for using functional options across the library, and in particular for instantiating a Realtime or REST client since that is going to be used by all users of this library.

It seems we are in disagreement about the use of a struct to contain the functional options versus having them be functions on the package (e.g. see #141 (comment), #145 (comment) and #202 (comment)), which for ClientOptions is a decision between the following two approaches for instantiating a client with a key or a token:

1. struct container:

ably.NewRealtime(ably.NewClientOptions("<key>"))

ably.NewRealtime(ably.ClientOptions{}.Token("<token>"))

2. package functions (I'm on the fence about the With prefix, but I know some use it as an indication that it's an option):

ably.NewRealtime(ably.WithKey("<key>"))
ably.NewRealtime(ably.Key("<key>"))

ably.NewRealtime(ably.WithToken("<token>"))
ably.NewRealtime(ably.Token("<token>"))

To reply to your concern raised in #141 (comment):

The problem with that approach is that we have a lot of options, so putting them all in the same top-level namespace is problematic for discoverability and potential name conflicts.

I personally think the improved user-facing API I propose outweighs the problems you mention.

To give an example, the popular grpc package has 40 dial options, all defined as functions on the package that return DialOption, all grouped under the declaration of DialOption in godoc so easily discoverable, and avoiding name conflicts by being prefixed with With (although CallOption from the same package doesn't use the With prefix and still avoids conflicts).

@gernest @paddybyers do you have any opinions on this?

[paddybyers]

do you have any opinions on this?

I am concerned about the potential for name conflicts if we have multiple APIs with variadic arguments of this type.

That said, I think the scope of the API isn't going to change that much, and the number of types, and the opportunity for conflict is limited, so I would say we should go with whatever we think is the most idiomatic.

[tcard]

@lmars

• there is a ChannelOptions type but the methods that I assume it relates to (i.e. Channels.Get and RestChannels.Get) take ...ChannelOption, this seems incorrect?

There's some rationale at #146. But then, it was just an ad-hoc design at the time. We should revisit it now.

By strictly following what's been discussed here, there should be a GetWithOptions and then an option like WithChannelOptions(ChannelOptions). That seems redundant. I think we should "collapse" the ChannelOptions as if each channel option were an optional parameter to Get. Does this sound good?

• given we've renamed RestChannel to RESTChannel, should we also rename RestChannels to RESTChannels and RestPresence to RESTPresence (although the spec does reference RestChannel, so perhaps this is going off-spec)?

Done at c57b530 and 6febe35 (part of ably/specification#83).

should the following take a context.Context?

• should REST.Request and Realtime.Request have WithOptions variants as per the discussion above?

Yes, for sure, it's just pending. ably/specification#83 implements the subset we talked about so that users can start consuming the new API with some confidence: connection and channel events, message and presence publish and subscribe.

---

It seems we are in disagreement about the use of a struct to contain the functional options versus having them be functions on the package

Just for clarification: it isn't a struct, it's a slice type of which each method appends an option and returns the resulting slice.

I am concerned about the potential for name conflicts if we have multiple APIs with variadic arguments of this type.

We can (and really should) add a prefix for options that aren't ClientOptions.

I don't really feel strongly about keeping the slice design, and given you do, let's go that way.

[tcard]

I've incorporated all this into the "Options" and "Optional arguments" in the issue text. @lmars @gernest @paddybyers Please take a look.

[lmars]

@tcard

I think we should "collapse" the ChannelOptions as if each channel option were an optional parameter to Get. Does this sound good?

Assuming you mean:

channels.Get("name", ably.WithChannelCipher("..."))

channels.Get("name", ably.WithChannelCipherKey("..."))

Sounds good to me.

Done at c57b530 and 6febe35 (part of ably/specification#83).

Nice 👍

Yes, for sure, it's just pending

Ok, is there something tracking that? Just want to make sure we don't miss anything.

I don't really feel strongly about keeping the slice design, and given you do, let's go that way.

Ok great, so is the next step a PR with the options changed? Happy to help with that if needed, just let me know.

[tcard]

About history API, by @lmars from #206 (comment):

The usage feels a little awkward, I wonder if something like the following would be better:

params := &ably.PaginateParams{
	Limit: 10,
}
err := channel.Presence.Get(params, func(page *ably.PaginatedResult) bool {
	// iterate page.PresenceMessages()
	// return false to stop iterating
})

and similarly for the other paginated operations.

[ably-sync-bot]

➤ Lewis Marshall commented:

I think this discussion has been had and agreements made, let’s open new tickets if anything else comes up.