Moved on operator to Signal

[NachoSoto]

Note that Signal.on is equivalent to SignalProducer.on except for the started event, which wouldn't make sense on Signal.

I also added a test to verify that disposed is invoked at the right time.

[NachoSoto]

Flaky test is flaky. The only failing test is the one in RACSequenceSpec which is unrelated to this change.

[neilpa]

I don't understand why we need this for Signal. It doesn't make sense to "inject" effects on a signal. It's already hot and you can just observe it. If the goal is to have an onDisposed handler (which I'm not sure there's much value in) that should be an isolated thing.

[NachoSoto]

For the most part that's true. However, sometimes you do want to synchronize side effects across multiple subscriptions, and you can't do that with multiple subscriptions!

Example:

originalSignal
  .observeNext(sideEffects1)

originalSignal
  .skipRepeats()
  .flatMap(.Latest, transform: f)
  .observeNext(sideEffects2)

If you need to guarantee that sideEffects1 always precedes sideEffects2 the above code is fragile.
Instead, one should do this:

originalSignal
  .on(next: sideEffects1)
  .skipRepeats()
  .flatMap(.Latest, transform: f)
  .observeNext(sideEffects2)

So that's why I did this.

[neilpa]

That's true, but is this really a pattern we should encourage by adding it to the public API? My gut says no given the nuances with trying to explain the differences between observe and on.

[NachoSoto]

given the nuances with trying to explain the differences between observe and on.

That's an important difference, for sure, but given that the semantics are the same as in SignalProducer between on and start (ignoring the differences between observe and start), and the fact that it's a common operator in ReactiveExtensions, I don't think it's a concern.

I don't know about it being a pattern we should encourage, but it's certainly something necessary.
To put my example in words:
You have a stream of values. From this stream, you're making an HTTP request for every value (say using .Merge). You want to perform side effects for every emitted value, as well as for every result emitted by the network request.

Without this addition there is no way to do this in a pure way without a potential risk for race conditions.

[neilpa]

given that the semantics are the same as in SignalProducer between on and start (ignoring the differences between observe and start)

There's a relation, but we can't ignore the difference between observe and start here because that's the entire point of on. It attaches effects to a producer that run each time it's started. This doesn't correlate to signals in quite the same way that other operators do.

it's a common operator in ReactiveExtensions

But they don't distinguish between hot and cold signals.

From this stream, you're making an HTTP request for every value (say using .Merge). You want to perform side effects for every emitted value, as well as for every result emitted by the network request.

The most common effect being logging which I'd feel comfortable adding. Anything else and you're probably better served by trying to integrate whatever you're doing into the signal pipeline functionally. This just doesn't feel like a problem we should attempt to solve given the other problems around complexity and understanding it introduces.

@ReactiveCocoa/reactivecocoa Any other opinions?

[NachoSoto]

But they don't distinguish between hot and cold signals.

I know, but this is something that users of Rx still need to keep in mind. Using on is not necessarily doing anything if there are no clients of the stream. Just saying that this is not that hard to understand for newcomers.

Anything else and you're probably better served by trying to integrate whatever you're doing into the signal pipeline functionally.

How would you suggest implement what I described "integrating it into the signal pipeline functionally"? If you give me an alternate implementation, then I completely agree with everything you're saying. I'd rather not add a new operator if don't have to, but the case I'm trying to make is that it does solve a real problem.

The most common effect being logging which I'd feel comfortable adding.

That's another example of something that users will want to do very often:

signal
  .someOperator()
  .on(next: { print("Log 1: \($0)") })
  .flatMapErrors( ... )
  .on(error: { print("Error: \($0)") })
  .otherOperator
  .on(next: { print("Log 2: \($0)") })

You can accomplish this by separating the signal into intermediate values (because in this case, potential race conditions don't really matter that much I suppose), but that's VERY annoying in practice and this operator would make it a lot easier to do.

[neilpa]

How would you suggest implement what I described "integrating it into the signal pipeline functionally"? If you give me an alternate implementation

I'm not sure what you're scenario is but it seems like behavior that should be captured in a custom operator and not a generic escape hatch.

That's another example of something that users will want to do very often

but that's VERY annoying in practice and this operator would make it a lot easier to do.

How often are you actually doing this? Outside of logging (which I've made custom operators for) I've not had a need for this. I'd much prefer to solve the logging issue directly and not add another operator that's nearly the same as observe

[jspahrsummers]

I agree with @neilpa's objections, but at the same time, it seems weird to offer an operator for SignalProducer that would also be (mostly) valid for Signal, and not provide the latter. I think we should add it.

[NachoSoto]

Especially since it solves a real problem. @neilpa you haven't answered my questions: if you can come up with a way to solve the issue above, then maybe there is no need for this, but otherwise this is actually useful. You can see the "real-world" example that made me add this: NachoSoto/AsyncImageView@3981351

With the double subscription, since the producer returned by flatMap can emit synchronously, there was a race condition (and it was easily reproducible because, the image view would be empty!). This change allows a user to expression that sort of flow (logging intermediate values being another example) in a more correct way.

[neilpa]

I see, you're deriving from UIImageView which limits your flexibility. I'm not sure why you need to clear the image on every incoming request though instead of waiting for the result. But given that requirement and the derivation I don't see an alternative.

I still think this is a niche operator and would prefer to just solve the logging problem. But it looks like I'm in the minority here.

[jspahrsummers]

[neilpa]

I'm sorry Toby