Add no-argument form of combinePrevious(), where previous value is optional

[sharplet]

I personally found this operator useful when filtering a stream, such that the first value should always be passed through, but subsequent values should be compared to previous values. For example:

producer.combinePrevious()
  .filter { previous, next in
    guard let previous = previous else { return true }
    return previous.value != next.value
  }
  .map { $1 }

[NachoSoto]

Thanks for the PR! There are 3 issues with this, however:

• One of the combinePrevious implementation is not implemented in terms of the other.
• The closure passed to map should not have side effects.
• This operator is redundant: the existing combinePrevious is more general. You can accomplish the same thing by doing something like producer.combinePrevious(nil).skip(1).

[NachoSoto]

There is a similar discussion in #2561.

[sharplet]

You can accomplish the same thing by doing something like producer.combinePrevious(nil).skip(1).

Unfortunately this doesn't compile, as the type of producer is initially non-optional, thus nil isn't a valid initial value. My first attempt at solving this problem used the existing combinePrevious(_:), and looked like this:

producer
  .map(Optional.init) // allow nil to be a valid initial value
  .combinePrevious(nil)
  .filter { previous, next in
    guard let next = next else { fatalError() } // not ideal
    guard let previous = previous else { return true }
    return previous.value != next.value
  }
  .map { $1 }
  .ignoreNil() // this is necessary because we had to lift the whole stream into Optional at the start

There's a few reasons this isn't ideal, but the main one for me is how much this version obscures the intent, because of the dance around Optional.

One of the combinePrevious implementation is not implemented in terms of the other.

Yeah, perhaps this is actually overloading the name combinePrevious with something too different? However it still feels to me like it's in the same spirit, even though the types are different.

The closure passed to map should not have side effects.

Would you mind elaborating on this? Totally get there could be a latent problem here, I'm just having difficulty reasoning about it in my head.

[NachoSoto]

Unfortunately this doesn't compile, as the type of producer is initially non-optional

Sorry, I was typing from my phone and I missed that. In any case, you can easily add this as an extension in your project. I'm not in favor of adding an overload to combinePrevious with different semantics. The goal for now is to get RAC 4's stable, with a minimum API surface. Since the behavior that you want can be easily created using existing operators I'd say that it doesn't add much value for now.

[sharplet]

Thanks for taking the time to review! 🙇

I'll go ahead and close this.

If you get a chance, I'd love to know more about the closures with side effects issue. I had trouble working out if there's anything in the Design Guidelines addressing this particular issue.

[sharplet]

FWIW, I did some investigation on whether it's a problem for the block to have side effects in this case. I'm not sure if my reasoning is 100% correct, but I thought it might be useful to throw this out there.
If you have an opportunity to take a look at this @NachoSoto I'd really appreciate it!

Here's a simplified object graph for the SignalType version of the operator:

Signal (underlying) <--- closure passed to `map()` <-- Signal (combinePrevious)
                      /
       `next` var <--/

Because the closure has a reference to the next var, if multiple objects gained a reference to the closure then the effect would be shared amongst all of them.

However, I don't think this is a problem in this case, because there's no way to copy a Signal.

If the same implementation was implemented for SignalProducerType, that would be a problem. Signal producers are easily copied, and so any effects are shared between the different copies.

SignalProducer (underlying) <--- closure passed to `map()` <-- SignalProducer (combinePrevious)
                              /                          ^
               `next` var <--/                            \--- SignalProducer (copy)

In this case the next var is effectively shared between multiple producers. This effectively breaks the value semantics.