Various spec fixes

[mattheworiordan]

@tcard @paddybyers @SimonWoolf a few small fixes for issues created recently.

[SimonWoolf]

👍

Re syncComplete -- it's currently a function in ably-js, and I'm inclined to leave it as a function in 0.9, I don't think the change is worth breaking backwards compatibility over. (And we can't make both ways work, functions are truthy). WDYT?

[mattheworiordan]

Re syncComplete -- it's currently a function in ably-js, and I'm inclined to leave it as a function in 0.9, I don't think the change is worth breaking backwards compatibility over. (And we can't make both ways work, functions are truthy). WDYT?

Well perhaps, I suppose at some point we need to fix the inconsistency and I very much doubt many people are using it, so I'd prefer to avoid later pain by taking the pain now. But up to @paddybyers and @tcard perhaps to chime in.

[tcard]

I already said somewhere else that I'd put in the spec that methods with no arguments and non-IO return types can be implemented as attributes as well (and the other way around). I don't think there's much harm in giving a little leeway there.

[mattheworiordan]

I already said somewhere else that I'd put in the spec that methods with no arguments and non-IO return types can be implemented as attributes as well (and the other way around). I don't think there's much harm in giving a little leeway there.

Well in non-typed languages that can lead to confusion, especially if people are doing if (presence.syncComplete) { ... }

[SimonWoolf]

I suppose at some point we need to fix the inconsistency

I agree that if we're planning on changing it at some point we should do it as early as possible (i.e. now). What I was suggesting was leaving it as a function forever -- aside from not breaking peoples code now (there're no compilation failures to tell people something's changed when we push out a new ably-js to the cdn), having things as functions gives us more flexibility in the future should we want to deprecate it or do other things with it. Getters have some advantages over attributes for languages that aren't as flexible as ruby.

if people are doing if (presence.syncComplete) { ... }

That's a general argument against any functions whose name doesn't completely disambiguate whether they're functions or attributes -- do we really want a worry that people might mistake functions for attributes to drive our api design? Surely at some point we have to assume that people are either reading the docs or examining things in a repl, or we'll end up having to call everything syncCompleteFunctionThatReturnsABoolean etc...

[mattheworiordan]

I don't feel strongly enough one way or the other to be honest so would be happy to go with a function. However, I do think that syncComplete is really just a state, and all state fields are attributes as opposed to functions, so if we applied the same thinking then channel.state should be a function too. In fact in Ruby my RealtimePresence members map object has a state, see https://github.com/ably/ably-ruby/blob/4008873524411d368321bd489fff90e9118c3132/lib/ably/realtime/presence/members_map.rb#L21-L28. I'm not proposing we introduce that, but just saying that the attribute is analogous to a state.

do we really want a worry that people might mistake functions for attributes to drive our api design?

We do use attributes already so not sure I understand the point. This discussion was specifically around whether we should be changing a function in JS to an attribute and the implications of that.

As above though, I don't feel strongly enough about it one way or the other, so let's just vote.

Matt +1 for an attribute (rationale being that it's just a state attribute like errorReason or state)

[paddybyers]

Matt +1 for an attribute (rationale being that it's just a state attribute like errorReason or state)

I tend to agree; ie you'd have an attribute or field unless there was an overriding reason on a given language for it to be different.

[tcard]

I think we all agree an attribute is generally better (and specifically in this case). Shouldn't the discussion be about:

• Whether we should allow for "pure values" to be either attributes or returned by methods with no args, at the implementor's discretion.
• Relatedly, whether, in ably-js, we should make the breaking change of syncComplete: () -> bool to syncComplete: bool.

[SimonWoolf]

👍 to @tcard's point. I agree that if writing it from scratch we'd go for an attribute, but I'm pretty wary of pushing an update to the CDN that changes it from one to the other. If anyone is using it, that'd just be gratuitously breaking people's code out from under them. (My point here was just that, given that, leaving it as a function wouldn't be so bad)

Quick thought -- we could add an endpoint to something or other that just does a rollbar if called. I could then push an update to ably-js 0.8 that does an http request to that endpoint (with the app id) anytime anyone calls syncComplete(). If we get no calls to that endpoint, we'll have proof that no-one's using syncComplete at the moment and we can change it. If we do, then we get a list of apps that are using that endpoint, so we can make a decision based on that, and if we decide to change we know who to contact and tell them they have to change. WDYT?

[paddybyers]

Whether we should allow for "pure values" to be either attributes or returned by methods with no args, at the implementor's discretion.

I think yes, and I think that was my comment, with the emphasis that there should need to be a reason to do it for that language (idiom or whatever) rather than simply the personal taste or preference of the implementor.

whether, in ably-js, we should make the breaking change of syncComplete: () -> bool to syncComplete: bool.

In this case I think the downsides of changing it outweigh the benefits.

[mattheworiordan]

whether, in ably-js, we should make the breaking change of syncComplete: () -> bool to syncComplete: bool.

In this case I think the downsides of changing it outweigh the benefits.

The problem is made a bit more difficult because of the docs too. We will now need to modify the docs so that we have syncComplete as an attribute for all languages at http://docs.ably.io/realtime/versions/v0.8/presence/#sync-complete, and then move it to the methods section below that for JS. I really dislike that.

In this case I think the downsides of changing it outweigh the benefits.

We have a list of many breaking changes in 0.9, see #235. I don't understand why this change is considered more significant than all the other breaking changes that we need to communicate to people anyway? Surely if we're going to tell people that 0.9 is a breaking change, then we should just bundle this in so as to maintain consistency in our libs (which is a core USP we offer to customers).

[mattheworiordan]

BTW. As this state attribute does not emit it's state as an event, then I very much doubt people are using this attribute anyway as I can't really see how people would use it. If they wanted to wait until the presence set was in sync, would they set up a timer that keeps checking this attribute value? I don't think so, I think they'd simply call get and wait.

[SimonWoolf]

I don't understand why this change is considered more significant than all the other breaking changes that we need to communicate to people anyway?

In ably-js, we've gone to some efforts to make the changes as non-breaking as possible (soft-deprecated old methods, allowing old parameters patterns with deprecation warnings, etc.) -- ISTM the standards for non-breaking-ness are a lot higher for cdn.ably.io/lib/ably.js than for e.g. server libs (especially complied ones): we want people to trust that we won't break ably.js, because if they don't, they'll lock into minor or even patch versions, which will be worse for us.

I very much doubt people are using this attribute

Any thoughts on my idea of getting some hard data on that, so we don't have to guess? see prev comment

[mattheworiordan]

In ably-js, we've gone to some efforts to make the changes as non-breaking as possible

Yup, and we should do this in all languages. But there are breaking changes in auth, new channel states, new channel events, new behaviour for presence and channel recovery, new ways to know if continuity is lost, implicit attach now registers a listener on failure when it didn't before. The things we've done to make things as non-breaking as possible does not make any difference in these very significant changes.

I very much doubt people are using this attribute

Any thoughts on my idea of getting some hard data on that, so we don't have to guess? see prev comment

I don't see the point given above. I also don't know how we'd know conclusively given people use Node.js and bundle the lib as well, and we'd also need to add Rollbar to all previous versions of the library as well which could introduce a new breaking change to all existing libraries. So I like the idea, but I don't think we should do it.

Given 0.9 is breaking (regardless of what we've done to smooth things over), how about we release it as 1.0 instead now? There are many reasons why that is a good idea, including making it clear Ably has a stable release for our libs (0.x is often considered pre-release) and also making it clear to customers an upgrade from 0.x to 1.0 is probably breaking and they should read the release notes. WDYT?

[SimonWoolf]

I also don't know how we'd know conclusively given people use Node.js and bundle the lib as well ... also making it clear to customers an upgrade from 0.x to 1.0 is probably breaking and they should read the release notes

People using nodejs (or any other library, or who are locking into a minor version on the CDN) I'm not worried about. They can upgrade at their leisure, read release notes, run their test suites, whatever. My worry is about people using ably.js on the CDN. They don't get any of that. They won't see any version change. We could change the version to 100.0.0 for all they'd know.

And we want people to keep using ably.js on the CDN! We want people to trust that as much as possible we won't break ably.js, because if they don't, they'll lock into minor or even patch versions, which will be worse for us.

there are breaking changes in auth, new channel states, new channel events, new behaviour for presence and channel recovery, new ways to know if continuity is lost, implicit attach now registers a listener on failure when it didn't before

I'm not sure any of those are equivalent. Different behaviour is one thing, a change that is guaranteed to raise an exception for anyone who was using a feature before ("TypeError syncComplete is not a function") is another, surely. (The breaking changes in auth we've made, as far as possible, non-breaking.)

we'd also need to add Rollbar to all previous versions of the library as well which could introduce a new breaking change to all existing libraries

I'm not suggesting we should add rollbar to the library. I agree that'd be crazy. I'm suggesting we add one line (that fires off an http request as part of syncComplete). Rollbar would on the endpoint serverside. (I can just add to translator, so we don't need a realtime deploy).

Not release a new version, not upload to npm, literally just upload to the CDN as ably.js, that's the only one I want data on. Probably just for 24 hours, then replace with the old version again.

If (as expected) we get no hits, I'll happily change to an attribute.

[mattheworiordan]

And we want people to keep using ably.js on the CDN! We want people to trust that as much as possible we won't break ably.js, because if they don't, they'll lock into minor or even patch versions, which will be worse for us.

I would argue we made a mistake in fact ever allowing people to access the library as http://cdn.ably.io/lib/ably.min.js and we're now letting that affect our decisions moving forwards.

We know that in future, we will release breaking changes, and customers should choose to opt in by upgrading or not.
By allowing customers to use http://cdn.ably.io/lib/ably.min.js, we're forcing ourselves into this horrible place where we're worried about updates because we want to ensure backwards compatibility. As far as I can tell, we just did this wrong at the ouset and we should have only allowed customers to use cdn.ably.io/lib/ably.min-0.9.js or cdn.ably.io/lib/ably.min-0.js, both of a customer could rely on to not break things for them. I appreciate 0.* according to Semver does not guarantee this, but for all major versions onwards it does.

So I think we'd be making a big mistake now by a) releasing an update to to cdn.ably.io/lib/ably.min.js that is breaking, b) allowing that CDN library to dictate what we do moving forwards.

I propose we therefore:

• Change 0.9 release to 1.0
• Leave http://cdn.ably.io/lib/ably.min.js locked to 0.8 and add a deprecation warning for customers now.
• Change syncComplete function to an attribute knowing that firstly customers will get an exception now when calling it (so it least it does not fail silently), and secondly it's a breaking change just like the rest of the 1.0 functionality.

Different behaviour is one thing, a change that is guaranteed to raise an exception for anyone who was using a feature before ("TypeError syncComplete is not a function") is another, surely.Different behaviour is one thing, a change that is guaranteed to raise an exception for anyone who was using a feature before ("TypeError syncComplete is not a function") is another, surely.

No, you think that it's acceptable that the following breaks from 0.8 to 0.9?

channel.on('detached', function() { 
  recoverMyLostMessagesAsDataConsistencyMatters();
});

I don't think we should decide what we think is an important breaking change or not. Customers should decide and as a result, we should let them choose to upgrade when they are ready and let them read the change release notes.

If (as expected) we get no hits, I'll happily change to an attribute.

I still don't think it matters personally given above.

[mattheworiordan]

This is now merged in 2d32107