Add withLatest(from:) (Rx.withLatestFrom) #128

inamiy · 2016-11-28T11:39:05Z

This PR is RAC port of Rx.withLatestFrom (namely, sample(from:)) with samplee's error type constrained to NoError only.

Diagram: http://rxmarbles.com/#withLatestFrom

This was originally discussed in ReactiveCocoa/ReactiveCocoa#2082 (not accepted), but I found this operator very useful because it is easy to fetch current state (MutableProperty) via sourceSignal:

let state: MutableProperty<String> = ...

sourceSignal
    .sample(from: state.producer)
    .observeValues { sourceValue, currentState in ... }

Above code is much easier to read compared to:

sourceSignal
    .map { [unowned state] sourceValue in (sourceValue, state.value) }
    .observeValues { sourceValue, currentState in ... }

or:

state.producer
    .sample(with: sourceSignal)
    .observeValues { currentState, sourceValue in ... }
// receiver is flipped, and hard to understand which signal is the source of this whole chaining

Please note that sample(from:) is NOT actually a flipped version of sample(with:) since it discards samplee's any terminal events.
For .completed handling, this behavior is same as Rx.withLatestFrom (and is often easy to use).

The code is originally from: https://github.com/inamiy/ReactiveAutomaton/blob/28027a656b923008f34089c739e51cdbe13b65cb/Sources/ReactiveCocoa%2BSampleFrom.swift

sharplet · 2016-11-28T12:59:08Z

Do you have an example with more meaningful names than sourceSignal and state? I think that would help to demonstrate the readability benefits.

inamiy · 2016-11-28T14:09:47Z

@sharplet
Button with "2 enabled flags" can be a good example:

let enabled1 = MutableProperty<Bool>(true) 
let enabled2 = MutableProperty<Bool>(true) 
let buttonTap: Signal<(), NoError> = ...

buttonTap
    .sample(from: enabled1.producer)
    .sample(from: enabled2.producer)  // this signal is `Signal<((Void, Bool), Bool), NoError>`
    .filter { $0.1 && $1 }
    .observe { _ in print("hello") }  // prints only when both `enabled1` & `enabled2` are true

I think this is hard to implement if only existing sample(with:) is allowed to use.

mdiep · 2016-11-28T16:53:33Z

That example seems like it'd be better with sample(on:).

let enabled1 = MutableProperty<Bool>(true) 
let enabled2 = MutableProperty<Bool>(true) 
let buttonTap: Signal<(), NoError> = ...

SignalProducer
    .combineLatest(enabled1.producer, enabled2.producer)
    .sample(on: buttonTap)
    .filter { $0 && $1 }
    .observe { _ in print("hello") }  // prints only when both `enabled1` & `enabled2` are true

inamiy · 2016-11-28T21:06:07Z

@mdiep
Oh, your code works very nice 😅
How about asynchronously fetching 2 states, i.e. sample(with:).flatMap().sample(with:)?

/// Stores API response items.
let items = MutableProperty<[Item]>([])

items <~ buttonTap
    .sample(from: urlTextField.reactive.text)
    .map { APIRequest($1) }
    .flatMap(.merge) { sendRequest($0) }
    .sample(from: items.producer)
    .map { $1 + [$0.item] }

items.signal
    .observeValues { print("items updated: \($0)") }

inamiy · 2016-11-28T21:16:49Z

Anyway, the dominant signal of above cases is buttonTap which is what I mean by sourceSignal (triggering signal). Placing it at topmost of the data flow (receiver) is more readable.

mdiep · 2016-11-28T21:29:46Z

I definitely think the sample(from:) name is wrong. Similarities exist with the sample(on:) and sample(with:), but I think the defining characteristic of sample is that it's source.sample(_: trigger). The name totally trips me up.

withLatest seems like a much clearer name.

But your example still doesn't seem ideal. In particular, I wouldn't use items in its own definition.

/// Stores API response items.
let items = MutableProperty<[Item]>([])

items <~ urlTextField.reactive.text
    .sample(on: buttonTap)
    .map { APIRequest($1) }
    .flatMap(.merge) { sendRequest($0) }
    .reduce([]) { $1 + $0 }

items.signal
    .observeValues { print("items updated: \($0)") }

sharplet · 2016-11-28T21:44:00Z

And with the recent improvements to Action, you can take a different approach without any mutable state:

let url = Property(initial: nil, then: urlTextField.reactive.text)

// Only enabled when input url is non-nil
let submit: Action<(), [Item], Error> = Action(input: url) { url in
  // url is the current value when the button is tapped
  let request = APIRequest(url)
  return sendRequest(request)
}

// bind button to submit action:
//   - button is disabled while executing
//   - button is disabled if input is invalid
button.reactive.pressed = CocoaAction(submit)

let items = Property(
  initial: [],
  then: submit.values.scan([]) { $1 + $0 }
)

items.signal
  .observeValues { print("items updated: \($0)") }

// you can also handle errors gracefully by observing `submit.errors`

inamiy · 2016-11-29T01:03:29Z

@mdiep
Renaming to withLatest(_:) is good to me too.
It's a cousin of sample(with:), but not just a flipped version anymore.

By the way, your code has difficulty when items.value is updated elsewhere other than buttonTap.
reduce causes unnecessary extra state inside.

@sharplet
Same here, scan is unnecessary internal state.
(and Action is overkill for just achieving map { ($0, stateProperty.value) })

sharplet · 2016-11-29T01:28:08Z

Placing it at topmost of the data flow (receiver) is more readable.

Now that you phrase it in terms of data flow, I see your point. This flows pretty well for me at the point of use:

items <~ buttonTap
  .withLatest(from: textField.reactive.continuousTextValues)
  .flatMap(.latest) { api.getItems($1) }
  // etc.

mdiep

I'm 👍, but it needs some changes and also requires tests.

mdiep · 2016-11-29T12:46:03Z

Sources/Signal.swift

+	///            sampled (possibly multiple times) by `self`, then terminate
+	///            once `self` has terminated. **`samplee`'s terminated events
+	///            are ignored**.
+	public func sample<U>(from samplee: Signal<U, NoError>) -> Signal<(Value, U), Error> {


I think withLatest(_ samplee: Signal<U, NoError>) would be clearer.

Alternative: combineLatest(from:).

Edit: IMO with is not directional, but from implies a direction.

This is different from combineLatest since it only sends a new value when one of the signals changes. So it should have a different name.

I prefer withLatest(_:) because "with" sounds like a small addition to the main stream, and receiver remains as the first class that also implies the rank direction.
Maybe withLatest(from:) can be a compromise, but I think we don't need this "from".

withLatest(_:) should be fine.

withLatest(textField.reactive.continuousTextValues)

reads like "with latest text values" to me. Edit: That's said the from label would give a stronger implication of "with the latest (one) from".

mdiep · 2016-11-29T12:47:44Z

Sources/Signal.swift

+	///            sampled (possibly multiple times) by `self`, then terminate
+	///            once `self` has terminated. **`samplee`'s terminated events
+	///            are ignored**.
+	public func sample<U>(from samplee: SignalProducer<U, NoError>) -> Signal<(Value, U), Error> {


This should be removed. We don't expose mixed operators like this. If someone wants to use a Signal with a SignalProducer, they need to convert the Signal to a SignalProducer.

We already have mixed operators like:

combineLatest<U>(with other: SignalProducer<U, Error>)

combineLatest<U>(with other: Signal<U, Error>)

flatMap<U>(Value -> SignalProducer<U, Error>)

flatMap<U>(Value -> Signal<U, Error>)

I think there are also needs for withLatest(signal) case too.

We do not have hot + cold -> hot except for flattening operators though. Only on SignalProducer we have overloads that take Signals directly.

Is it illegal to have different type for the argument?
I think hot -> T -> hot or cold -> T -> cold keeps the same structure, and it's good enough.
(This is probably the "endofunctor" thing in category theory after the partial application of T, though arguments are often flipped in Swift.)

It is not illegal. Just that hot -> cold -> hot doesn't seem something very commonly used, except for flattening operators (Signal<SignalProducer, E>).

mdiep · 2016-11-29T12:47:51Z

Sources/SignalProducer.swift

+	///            sampled (possibly multiple times) by `self`, then terminate
+	///            once `self` has terminated. **`samplee`'s terminated events
+	///            are ignored**.
+	public func sample<U>(from samplee: Signal<U, NoError>) -> SignalProducer<(Value, U), Error> {


This should also be removed.

This is for producer though. We have a ~~couple of~~ few cold -> hot -> cold producer operators.

mdiep · 2016-11-29T12:48:45Z

Sources/Signal.swift

+			}
+
+			return disposable
+		}


Can this be implemented with sample(with:)?

return samplee.sample(with: self).map { ($1, $0) }

To achieve the simplest map { ($0, stateProperty.value) } alternative as possible, I think returning signal should not get interfered with any of samplee's terminal events.

sample(with:), on the other hand, will wait for both sampler and samplee's .completed to finally emit its .completed, so this PR is not just a flipped sample(with:).

For comparison, in RxSwift, samplee's .completed is ignored but .failed is forwarded.

(As a side note, I have restricted samplee's error type as NoError only for now, allowing a future extension to also support samplee with non-NoError types when decision (ignore or forward error) is settled.)

andersio · 2016-11-30T15:54:08Z

(Nit)

// Button taps with (the) latest continuous text values.
buttonTaps.withLatest(textField.reactive.continuousTextValues)

// Button taps with (the) latest (one) from (the) continuous text values
buttonTaps.withLatest(from: textField.reactive.continuousTextValues)

sharplet · 2016-11-30T16:44:38Z

I agree, and think withLatest(from:) clarifies what "latest" refers to: the latest value from the signal.

inamiy · 2016-12-01T05:27:14Z

OK, it seems 2:2 now, but I can go for withLatest(from:) too.
If no other votes, I will rename it again later.
(BTW, is travis dead? Test is not working...)

andersio · 2016-12-01T05:51:36Z

Travis CI has had a huge backlog in Mac jobs since a day or two ago.

mdiep · 2016-12-01T13:15:29Z

I'm 👍 with withLatest(from:).

inamiy · 2016-12-01T23:49:40Z

Great! Renaming is done in 95bd754, and previous test has finally passed.

sharplet · 2016-12-02T04:56:19Z

Sources/Signal.swift

+	///         nothing happens.
+	///
+	/// - parameters:
+	///   - samplee: A signal that its latest value is sampled by `self`.


I think this would read better as "A signal whose latest value..."

Fixed in 9580123.

sharplet

LGTM

Add sample(from:)

8151aa8

mdiep requested changes Nov 29, 2016

View reviewed changes

inamiy added 2 commits November 30, 2016 22:42

Rename sample(from:) to withLatest(_:)

4082720

[Test] Add withLatest tests

5a8fa35

inamiy changed the title ~~Add sample(from:) (Rx.withLatestFrom)~~ Add withLatest (Rx.withLatestFrom) Nov 30, 2016

Rename withLatest(_:) to withLatest(from:)

95bd754

inamiy changed the title ~~Add withLatest (Rx.withLatestFrom)~~ Add withLatest(from:) (Rx.withLatestFrom) Dec 1, 2016

mdiep approved these changes Dec 2, 2016

View reviewed changes

sharplet reviewed Dec 2, 2016

View reviewed changes

mdiep added the enhancement label Dec 2, 2016

Fix comments in withLatest(from:)

9580123

sharplet approved these changes Dec 3, 2016

View reviewed changes

mdiep merged commit 9030845 into ReactiveCocoa:master Dec 4, 2016

inamiy deleted the feature/sample-from branch December 7, 2016 13:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add withLatest(from:) (Rx.withLatestFrom) #128

Add withLatest(from:) (Rx.withLatestFrom) #128

inamiy commented Nov 28, 2016

sharplet commented Nov 28, 2016

inamiy commented Nov 28, 2016 •

edited

Loading

mdiep commented Nov 28, 2016 •

edited

Loading

inamiy commented Nov 28, 2016

inamiy commented Nov 28, 2016 •

edited

Loading

mdiep commented Nov 28, 2016

sharplet commented Nov 28, 2016

inamiy commented Nov 29, 2016

sharplet commented Nov 29, 2016

mdiep left a comment

mdiep Nov 29, 2016

andersio Nov 29, 2016 •

edited

Loading

mdiep Nov 29, 2016

inamiy Nov 30, 2016

andersio Nov 30, 2016 •

edited

Loading

mdiep Nov 29, 2016

inamiy Nov 29, 2016

andersio Nov 29, 2016 •

edited

Loading

inamiy Nov 30, 2016 •

edited

Loading

andersio Nov 30, 2016

mdiep Nov 29, 2016

andersio Nov 29, 2016 •

edited

Loading

mdiep Nov 29, 2016

inamiy Nov 29, 2016

andersio commented Nov 30, 2016 •

edited

Loading

sharplet commented Nov 30, 2016

inamiy commented Dec 1, 2016

andersio commented Dec 1, 2016 •

edited

Loading

mdiep commented Dec 1, 2016

inamiy commented Dec 1, 2016

sharplet Dec 2, 2016

inamiy Dec 2, 2016

sharplet left a comment

Add withLatest(from:) (Rx.withLatestFrom) #128

Add withLatest(from:) (Rx.withLatestFrom) #128

Conversation

inamiy commented Nov 28, 2016

sharplet commented Nov 28, 2016

inamiy commented Nov 28, 2016 • edited Loading

mdiep commented Nov 28, 2016 • edited Loading

inamiy commented Nov 28, 2016

inamiy commented Nov 28, 2016 • edited Loading

mdiep commented Nov 28, 2016

sharplet commented Nov 28, 2016

inamiy commented Nov 29, 2016

sharplet commented Nov 29, 2016

mdiep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andersio Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andersio Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andersio Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

inamiy Nov 30, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andersio Nov 29, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andersio commented Nov 30, 2016 • edited Loading

sharplet commented Nov 30, 2016

inamiy commented Dec 1, 2016

andersio commented Dec 1, 2016 • edited Loading

mdiep commented Dec 1, 2016

inamiy commented Dec 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharplet left a comment

Choose a reason for hiding this comment

inamiy commented Nov 28, 2016 •

edited

Loading

mdiep commented Nov 28, 2016 •

edited

Loading

inamiy commented Nov 28, 2016 •

edited

Loading

andersio Nov 29, 2016 •

edited

Loading

andersio Nov 30, 2016 •

edited

Loading

andersio Nov 29, 2016 •

edited

Loading

inamiy Nov 30, 2016 •

edited

Loading

andersio Nov 29, 2016 •

edited

Loading

andersio commented Nov 30, 2016 •

edited

Loading

andersio commented Dec 1, 2016 •

edited

Loading