RTL10b: Implement untilAttach for RealtimeChannel.history. #246
Conversation
| @@ -16,4 +17,10 @@ ART_ASSUME_NONNULL_BEGIN | |||
|
|
|||
| @end | |||
|
|
|||
| @interface ARTRealtimeHistoryQuery () | |||
|
|
|||
| @property (strong, readwrite) ARTRealtimeChannel *realtimeChannel; | |||
There was a problem hiding this comment.
Maybe it's better to be weak.
There was a problem hiding this comment.
Hmm, why? realtimeChannel doesn't hold references to the ARTRealtimeHistoryQuery. There's no cycle here as far as I can see.
There was a problem hiding this comment.
The query can retain the channel if you create the ARTRealtimeHistoryQuery in other part of the main code and the client tries to release the channel.
There was a problem hiding this comment.
Right, but as soon as the query is gone (ie. as soon as the call site returns) the channel will lose its only reference left and all will be good. How can that leak?
There was a problem hiding this comment.
The user can hold the query anyway and with weak you will have the guarantee.
There was a problem hiding this comment.
I really can't see and scenario in which this changes anything in practice, because the only way this property will be set is by ARTRealtimeChannel.history itself, and if you're calling that method obviously you have the channel, and even if you drop the channel and hold to the query you can't do anything with the query but reuse it, which will change its realtimeChannel. But anyway, done at 547e39c.
|
Beside a small comment, LGTM |
| if (self.realtimeChannel.state != ARTRealtimeChannelAttached) { | ||
| @throw [NSError errorWithDomain:ARTAblyErrorDomain code:ARTRealtimeHistoryErrorNotAttached userInfo:nil]; | ||
| } | ||
| [items addObject:[NSURLQueryItem queryItemWithName:@"from_serial" value:self.realtimeChannel.attachSerial]]; |
There was a problem hiding this comment.
I believe this should be fromSerial as we changed all of our query params to use camelCase
There was a problem hiding this comment.
Why is it working then? It is from_serial in the spec.
There was a problem hiding this comment.
Because we support the old format as well for backwards compatibility :)
There was a problem hiding this comment.
Oh, OK. Fixed at 7532c73.
There was a problem hiding this comment.
There was a problem hiding this comment.
@ricardopereira That's done at 7532c73.
|
Looks good other than small comment |
tcard
force-pushed
the
history-until-attach
branch
from
7532c73 to
44e90f4
Compare
RTL10b: Implement untilAttach for RealtimeChannel.history.
Fixes #234.