Replace connectable signals with multicast connections #235

Merged
merged 12 commits into from Jan 2, 2013

Conversation

Projects
None yet
2 participants
@joshaber
Member

joshaber commented Jan 1, 2013

This replaces RACConnectableSignal with RACMulticastConnection, as discussed in #214.

I also removed RACCancelableSignal since it was a subclass of RACConnectableSignal and it seemed silly to spend time updating something that was going away. (#73)

There are a couple open questions still:

  1. In #214 we talked about dropping -autoconnect, but it seems like we still need something like that which disposes of the shared subscription automatically. I made changes to -autoconnect to make it safe compared to what it was.
  2. Should -replayLazily use -autoconnect? I'm leaning towards yes.

@joshaber joshaber referenced this pull request Jan 1, 2013

Closed

Added -memoize #214

@joshaber

This comment has been minimized.

Show comment Hide comment
@joshaber

joshaber Jan 1, 2013

Member

I went ahead and changed -replayLazily to use -autoconnect.

Member

joshaber commented Jan 1, 2013

I went ahead and changed -replayLazily to use -autoconnect.

@ghost ghost assigned jspahrsummers Jan 1, 2013

RACExtensions/NSTask+RACSupport.m
NSParameterAssert(scheduler != nil);
+ __weak NSTask *weakSelf = self;

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

This should be able to use EXTScope, since it's included in RAC proper.

@jspahrsummers

jspahrsummers Jan 1, 2013

Member

This should be able to use EXTScope, since it's included in RAC proper.

RACExtensions/NSTask+RACSupport.m
NSParameterAssert(scheduler != nil);
+ __weak NSTask *weakSelf = self;
+ return [RACSignal createSignal:^(id<RACSubscriber> subscriber) {

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

Shouldn't this be multicasted?

@jspahrsummers

jspahrsummers Jan 1, 2013

Member

Shouldn't this be multicasted?

+
+// Connects to the underlying signal when the returned signal is first
+// subscribed to and disposes of the subscription to the multicasted signal when
+// the returned signal has no subscribers.

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

We should make it very clear that this doesn't reconnect at any point.

@jspahrsummers

jspahrsummers Jan 1, 2013

Member

We should make it very clear that this doesn't reconnect at any point.

+- (RACDisposable *)connect;
+
+// Connects to the underlying signal when the returned signal is first
+// subscribed to and disposes of the subscription to the multicasted signal when

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

*to, and (just for readability)

@jspahrsummers

jspahrsummers Jan 1, 2013

Member

*to, and (just for readability)

+ RACSubject *_signal;
+}
+
+@property (nonatomic, readonly, strong) RACSignal *sourceSignal;

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

Can you please document what needs synchronization here?

@jspahrsummers

jspahrsummers Jan 1, 2013

Member

Can you please document what needs synchronization here?

This comment has been minimized.

Show comment Hide comment
@joshaber

joshaber Jan 2, 2013

Member

👍

+
+@interface RACMulticastConnection ()
+
++ (instancetype)connectionWithSourceSignal:(RACSignal *)source subject:(RACSubject *)subject;

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

Why not use an -init… method for this, if it's private anyways? It would make the ivar access nicer.

@jspahrsummers

jspahrsummers Jan 1, 2013

Member

Why not use an -init… method for this, if it's private anyways? It would make the ivar access nicer.

+
+#pragma mark Lifecycle
+
++ (instancetype)connectionWithSourceSignal:(RACSignal *)source subject:(RACSubject *)subject {

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

Should this assert that both arguments are non-nil?

@jspahrsummers

jspahrsummers Jan 1, 2013

Member

Should this assert that both arguments are non-nil?

+ if (!self.hasConnected) {
+ self.hasConnected = YES;
+
+ RACDisposable *sourceDisposable = [self.sourceSignal subscribe:_signal];

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

This doesn't need to be synchronized.

@jspahrsummers

jspahrsummers Jan 1, 2013

Member

This doesn't need to be synchronized.

+// signal to many subscribers. This is most often needed if the subscription to
+// the underlying signal involves side-effects or shouldn't be called more than
+// once.
+//

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

We should add another paragraph here, talking about how to activate the receiver's signal (and what it means to do so).

@jspahrsummers

jspahrsummers Jan 1, 2013

Member

We should add another paragraph here, talking about how to activate the receiver's signal (and what it means to do so).

This comment has been minimized.

Show comment Hide comment
@joshaber

joshaber Jan 2, 2013

Member

👍

-- (RACConnectableSignal *)multicast:(RACSubject *)subject;
+- (RACMulticastConnection *)multicast:(RACSubject *)subject;
+
+// Multicasts the signal to a RACReplaySubject and connects.

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

"connects immediately" might make it clearer that this has side effects.

@jspahrsummers

jspahrsummers Jan 1, 2013

Member

"connects immediately" might make it clearer that this has side effects.

+- (RACSignal *)replay;
+
+// Multicasts the signal to a RACReplaySubject. It will connect the multicast
+// connection on the first subscription.

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

This should clarify that it uses -autoconnect and inherits all the semantics of it.

@jspahrsummers

jspahrsummers Jan 1, 2013

Member

This should clarify that it uses -autoconnect and inherits all the semantics of it.

+
+// Multicasts the signal to a RACReplaySubject and connects.
+//
+// Returns the connection's signal.

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

This is the first reference to "the connection," and it might be unclear what it means.

@jspahrsummers

jspahrsummers Jan 1, 2013

Member

This is the first reference to "the connection," and it might be unclear what it means.

+// Returns the connection's signal.
+- (RACSignal *)replay;
+
+// Multicasts the signal to a RACReplaySubject. It will connect the multicast

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

*the RACMulticastConnection

@jspahrsummers

jspahrsummers Jan 1, 2013

Member

*the RACMulticastConnection

+
+- (RACMulticastConnection *)multicast:(RACSubject *)subject {
+ RACMulticastConnection *connection = [RACMulticastConnection connectionWithSourceSignal:self subject:subject];
+ connection.signal.name = [NSString stringWithFormat:@"[%@] -multicast: %@", self.name, subject];

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

This is equivalent to subject.name = …, and so will change the name of the subject given by the user. Maybe that's okay, but it could be confusing.

@jspahrsummers

jspahrsummers Jan 1, 2013

Member

This is equivalent to subject.name = …, and so will change the name of the subject given by the user. Maybe that's okay, but it could be confusing.

This comment has been minimized.

Show comment Hide comment
@joshaber

joshaber Jan 2, 2013

Member

Yeah though it'll include the original name as part of including subject in the name.

@joshaber

joshaber Jan 2, 2013

Member

Yeah though it'll include the original name as part of including subject in the name.

+- (RACSignal *)replay {
+ RACMulticastConnection *connection = [self multicast:[RACReplaySubject subject]];
+ [connection connect];
+ connection.signal.name = [NSString stringWithFormat:@"[%@] -replay", self.name];

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

Why not name the RACReplaySubject passed in, mirroring the pattern of -publish?

@jspahrsummers

jspahrsummers Jan 1, 2013

Member

Why not name the RACReplaySubject passed in, mirroring the pattern of -publish?

+ it(@"should return the same disposable for each invocation", ^{
+ RACDisposable *d1 = [connection connect];
+ RACDisposable *d2 = [connection connect];
+ expect(d1).to.equal(d2);

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

This test should also verify that subscriptionCount is still one.

@jspahrsummers

jspahrsummers Jan 1, 2013

Member

This test should also verify that subscriptionCount is still one.

This comment has been minimized.

Show comment Hide comment
@joshaber

joshaber Jan 2, 2013

Member

👍

+ expect(subscriptionCount).to.equal(0);
+});
+
+describe(@"-connect", ^{

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

Can you also add a test that verifies the behavior of -connect (same disposable returned, no new subscription) after the original subscription is terminated?

@jspahrsummers

jspahrsummers Jan 1, 2013

Member

Can you also add a test that verifies the behavior of -connect (same disposable returned, no new subscription) after the original subscription is terminated?

This comment has been minimized.

Show comment Hide comment
@joshaber

joshaber Jan 2, 2013

Member

👍

+ expect(subscriptionCount).to.equal(1);
+ });
+
+ it(@"should dispose of the multicasted subscription when the signal has no subscribers", ^{

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

It might be a bit redundant with this test, but I still think it'd be a good idea to verify that a new subscription doesn't automatically reconnect.

@jspahrsummers

jspahrsummers Jan 1, 2013

Member

It might be a bit redundant with this test, but I still think it'd be a good idea to verify that a new subscription doesn't automatically reconnect.

This comment has been minimized.

Show comment Hide comment
@joshaber

joshaber Jan 2, 2013

Member

👍

@jspahrsummers

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 1, 2013

Member

📶

Member

jspahrsummers commented Jan 1, 2013

📶

@joshaber

This comment has been minimized.

Show comment Hide comment
@joshaber

joshaber Jan 2, 2013

Member

🌸

Member

joshaber commented Jan 2, 2013

🌸

@jspahrsummers

This comment has been minimized.

Show comment Hide comment
@jspahrsummers

jspahrsummers Jan 2, 2013

Member

🍒

Member

jspahrsummers commented Jan 2, 2013

🍒

jspahrsummers added a commit that referenced this pull request Jan 2, 2013

Merge pull request #235 from github/multicast
Replace connectable signals with multicast connections

@jspahrsummers jspahrsummers merged commit fccc25f into master Jan 2, 2013

1 check passed

default Build #220421 succeeded in 16s
Details

@jspahrsummers jspahrsummers deleted the multicast branch Jan 2, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment