Fix attach callbacks, and add to RealtimeChannel.subscribe. #232
Conversation
| waitUntil(timeout: testTimeout) { done in | ||
| expect(channel.state).to(equal(ARTRealtimeChannelState.Initialised)) | ||
|
|
||
| channel.subscribeWithAttachCallback({ errorInfo in |
There was a problem hiding this comment.
Does this method need to be overloaded given we've "preferred" to stick with the overloaded IDL?
There was a problem hiding this comment.
There's already a subscribe:cb: method that takes a message name in its first parameter. Objective-C doesn't let you overload only by type, you need to either have different number of params, different names for them, or just take any type (id, the equivalent to Java's Object) and downcast it manually, which we don't do anywhere else and is strongly anti-idiomatic.
There was a problem hiding this comment.
Yes, @tcard is right. Swift conversion could be better with this. 😞 It should be imported by Swift like the initialisers where if the method takes an argument, the With is removed and the rest of the selector is divided up into named parameters accordingly.
|
This PR looks good, but should we not also have a scenario for calling attach when failed, or calling attach when channel becomes failed, or calling detach when failed, or calling detach when becomes failed to ensure the callback is called with an error in those cases? |
|
@mattheworiordan That's already done for |
Nope, that is different. A channel can be FAILED or become FAILED because of a permissions issue, whereas that test is for a connection state issue blocking the attach
Ok, well perhaps we should defer to later. I thought it would be easy enough to add given you simply need to copy & paste the current test and cause the channel to fail, using permissions is the easiest way |
|
@ricardopereira Could you take a look at this? It's blocking a couple of other changes. |
|
LGTM |
The callback was being subscribed to the message's ACK, while for
ATTACH and DETACH there's no ACK. Instead, an {AT,DE}TACHED
message is received. I've moved the callback call there, using
EventEmitter.
There was only a test for the attach timeout, which did work for
the subtle behavior that channel transitioning always cancels the
timeout. I've added tests for the happy paths as well.
Fix attach callbacks, and add to RealtimeChannel.subscribe.
Had to fix channel attach first. You can more easily review first one commit and then the other.
Fixes #220.