Spec validation

[mattheworiordan]

For version 0.8 of our client libraries, we have a documented specification at http://docs.ably.io/client-lib-development-guide/features/

It is required that all of our client libraries that use a 0.8.x version number, where possible, adhere to this specification ensuring feature compatibility across all languages and platforms. To ensure that we do not have regressions in future from this specification, all of the features within the 0.8 spec should have a matching test as well.

Please can you review this library against the feature set and do the following:

• Ensure that all spec requirements are implemented, I believe there are a few small variations
• Check that the README.md is up to date with the Ruby version with details such as how to install it from a package manager if applicable.

[mattheworiordan]

Matt to put a spreadsheet together based on current test status

[SimonWoolf]

Finished reviewing ably-js against the spec. Marked WIPs when I believe something should work but there's no test for it, but not guaranteeing all the wips until there are tests.

Here's list of everything I marked 'no' against, broadly sorted into: non-matching names, missing functionality, and non-matching functionality:

(while most things on the lists are bugs, I don't want to assume that every variance from the spec is a bug -- eg while the spec says that TokenRequest should be "a type", it's arguably idiomatic js to have it just be a plain object)

Wrong names for things

• RSC12,TO3k2 - host should be restHost
• RTC1d,TO3k3 - wsHost should be realtimeHost
• RTL6c2, RTP16b, TO3g - queueEvents option should be queueMessages
• RTP6a/b - presence#on should be presence#subscribe
• RTP7a/b - presence#off should be presence#unsubscribe
• TO3b/c - log: {level: n, handler: m} should be logLevel and logHandler

Missing functionality (in roughly descending order of importance)

• TO3e - no autoConnect option
• RTP11a/c1 - presence#get doesn't take a waitForSync param, it always waits for sync
• RTP13 - no Presence#syncComplete function (though there is #awaitSync)
• RSN3a/b/c,RTS3a/c,TB1 - can't provide channelOptions when instantiating a channel, need to call channel.setOptions
• TG6/7 - no PaginatedResult#hasNext or #isLast functions
• RTN11c - connect doesn't take a callback
• RTN16b/c - no Connection#recoveryKey attribute
• RSA8c,RSA8c1b,TO3j7 - no support for authMethod; assumes anything other than application/json is a string
• RSN4a - channels#release only exists for realtime channels, not rest
• TR4j - no ProtocolMessage#messageSerial attribute (it's msgSerial since that's what the raw protocol uses)
• TO3l3,TO3l6 - doesn't use httpOpenTimeout or httpMaxRetryDuration (rethink of timeouts & fallbacks in progress on Http fallback sequence tweaks #165)

Behaviour that doesn't match the spec (in roughly descending order of importance)

• RTP11b - presence#get doesn't raise for DETACHED or FAILED channels, it returns []
• RTP10d - presence#leave doesn't check if client is currently ENTERED
• RSA8c2 - authParams shouldn't take precedence over tokenParams in case of a clash
• CD2c/d/e/f - attributes maxMessageSize, maxFrameSize, maxInboundRate, connectionStateTtl of connectionDetails aren't checked
• RSA7c - client doesn't check clientId in clientOptions can't be * itself, leaves it to realtime to give an error
• RTP15f - relies on realtime to check clientId is compatible, doesn't check itself
• TE1,TD1,CD1 - TokenRequest, TokenDetails, ConnectionDetails aren't types, just plain objects

[paddybyers]

Thanks @SimonWoolf

TE1,TD1,CD1 - TokenRequest, TokenDetails, ConnectionDetails aren't types, just plain objects

These have no methods and therefore for JS I don't think creation of distinct types is necessary. Particularly for TokenRequest which can be user-constructed, there should not be an implicit expectation that they create an instance of a specific type.

RTP10d - presence#leave doesn't check if client is currently ENTERED

I'm pretty sure we decided that this was not appropriate, as there is no guarantee that the corresponding enter() occurred within the present library instance.

TR4j - no ProtocolMessage#messageSerial attribute (it's msgSerial since that's what the raw protocol uses)

That seems to have been unilaterally changed when the spec was written. There is no sense in these being different in the API and the protocol.

TO3b/c - log: {level: n, handler: m} should be logLevel and logHandler

I think this is an area where there will legitimately be differences between languages. IMO this seems more natural for a dynamic language, but logLevel, logHandler avoids the creation of a LogOptions type in the typed languages. If we add support for logLevel and logHandler we would presumably also continue to support log since we have lots of tests and probably other code out there that expects log.

RTP6a/b - presence#on should be presence#subscribe
RTP7a/b - presence#off should be presence#unsubscribe

Again, probably we now need to support both.

[mattheworiordan]

TE1,TD1,CD1 - TokenRequest, TokenDetails, ConnectionDetails aren't types, just plain objects

I agree with @paddybyers on this, I don't think types are necessarily needed. With Ruby, we do the same, and often interchangeably allow simple hashes.

I'm pretty sure we decided that this was not appropriate, as there is no guarantee that the corresponding enter() occurred within the present library instance.

I do recall discussing this, but thought we had decided that leaveClient would be used in those cases. I don't have a strong view on it, but I do find it odd that as enter/update/leave are designed to be used by the current client (and not a client with an anonymous clientId), they current client should not be leaving unless they entered. Will the server reject (NACK) the leave request if the client is not entered?

TR4j - no ProtocolMessage#messageSerial attribute (it's msgSerial since that's what the raw protocol uses)

That seems to have been unilaterally changed when the spec was written. There is no sense in these being different in the API and the protocol.

Yes, it seems to have been, shame it was not picked up sooner though as everyone has been working to this.

TO3b/c - log: {level: n, handler: m} should be logLevel and logHandler

I would agree with @paddybyers on this one, I think it's perfectly valid, especially for the logger, for it to be different based on the language implementation. For example, Python has a completely different way of approaching logging, so we support the idiomatic way instead.

RTP6a/b - presence#on should be presence#subscribe
RTP7a/b - presence#off should be presence#unsubscribe

Again, probably we now need to support both.

Fine, so long as the documented API of subscribe & unsubscribe is supported. However @paddybyers, if in future we introduced a normal EventEmitter on the Presence object for presence state changes, this could present problems for the Javascript API if we leave on in there now that also emits presence enter/update/leave events. Given we want to ensure our future API is not restricted and the fact that I don't think many people are using on, we should just go ahead and change this now.

[mattheworiordan]

Thanks @SimonWoolf for doing this!

[paddybyers]

TO3e - no autoConnect option

This has been implemented for a while:

066bf67

[paddybyers]

RTN11c - connect doesn't take a callback

(RTN11c) Where the language permits, a callback with a success and failure state can be provided

This requirement is unclear. What does the callback expect? Simple success/failure, or does the given callback get added as a ConnectionStateListener ? Or a listener for connected ? If a simple success/failure callback, it's too simplistic an API to be useful in real situations and provides an interface that's inconsistent with ConnectionStateListener. If the API accepts a ConnectionStateListener and adds it, that's then non-idiomatic and would give behaviour that may be unexpected.

[SimonWoolf]

TO3e - no autoConnect option

This has been implemented for a while: 066bf67

Oops. Sorry, yes - don't know why I marked TO3e 'no' when I marked RTC1b and RTN3 (the other specs relating to autoConnect) 'wip'. Fixed.

This requirement is unclear. What does the callback expect? ... If a simple success/failure callback, it's too simplistic an API to be useful in real situations ...

Good point. FWIW, current ably-ruby behaviour seems to be to call the callback for the first state change (only) after the connecting: it calls the block if that state is connected, errback if anything else. I agree that that seems nonideal (eg could get called with an err when goes into disconnected, but then successfully connect a few seconds later without telling the client).

Possibly a common case will be devs who won't care about the details of our connection retrying, but just want to know if either they're connected or it's irrecoverably failed, so maybe behaviour that calls back if connected and callsback with err if failed - only those two - might be useful?

[mattheworiordan]

current ably-ruby behaviour seems to be to call the callback for the first state change (only) after the connecting: it calls the block if that state is connected, errback if anything else

This is in fact a bug the Ruby library, and I should fix it. That was not the intention.

Possibly a common case will be devs who won't care about the details of our connection retrying, but just want to know if either they're connected or it's irrecoverably failed, so maybe behaviour that calls back if connected and callsback with err if failed - only those two - might be useful?

I would agree, providing a callback should simply be called when the intended state is reached, or the intended state will no longer be reachable without intervention i.e. if the state becomes CLOSED or FAILED. That makes sense to me.

[mattheworiordan]

@SimonWoolf FYI, my view on the issues is that other than the following, we should probably try and address this over the coming week or two:

• TO3b/c - log: {level: n, handler: m}. [MO] I think it's OK to leave this as is, it's specific to the lib
• RTP11a/c1 - presence#get doesn't take a waitForSync param [MO] The default of waiting is fine
• RTP13 - no Presence#syncComplete function [MO] Not sure it's really a high priority given you can just wait for sync with a get call
• RTN11c - connect [MO] Now removed from spec
• RSN4a - channels#release [MO] Future us problem, not high priority
• TR4j - no ProtocolMessage#messageSerial attribute [MO] Should no longer be an issue as correct the spec
• RTP10d - presence#leave doesn't check if client is currently ENTERED [MO] Very low priority
• RSA8c2 - authParams shouldn't take precedence over tokenParams in case of a clash [MO] This is an edge case problem, we can look at it later
• CD2c/d/e/f - attributes maxMessageSize, maxFrameSize, maxInboundRate, connectionStateTtl of connectionDetails aren't checked [MO] Not used now anyway, so not an issue
• RTP15f - relies on realtime to check clientId is compatible, doesn't check itself [MO] This works for now, so it's only an optimisation, defer to later
• TE1,TD1,CD1 - TokenRequest, TokenDetails, ConnectionDetails aren't types, just plain objects [MO] I think this is fine

[mattheworiordan]

I think this issue can be closed now as the tasks have been moved into separate issues #169, #170 and #171