Adding `lazyMap` #240

liscio · 2017-01-26T01:45:55Z


Most of this discussion is out of sync with this PR now, which merely adds `lazyMap` to `Signal` and `SignalProducer`, and a lazy value initializer to `SignalProducer` to support the construct.

The below text is retained merely for historical purposes.

---

This PR Includes `init(lazyValue:)` for `SignalProducer`, `lens` for `PropertyProtocol`, `lens` for `MutableProperty`, and a handful of associated tests to demonstrate that everything works as advertised.

For what it's worth, I think this can go into a 1.1 release if it is accepted. It's completely additive for 1.0 and doesn't change any existing API.

### Anticipated Questions

#### Q: Why is `bindingTarget` in `MutableProperty` and not `MutablePropertyProtocol`?

I used the `modify` function to ensure that the value is safely written back into the property by the setter. I don't think this is a major limitation, generally speaking.

Note: This will probably change once #182 is merged.

#### Q: How is this even useful? Shouldn't our model objects just have a `MutableProperty` for each of its properties?

I don't really think that's a good idea, and that approach has broken down for me very quickly, in practice. Model collections can mean thousands and thousands of objects or structs, and you certainly don't want a `MutableProperty` and its associated machinery replicated M x N times (where M is the # of properties, N the # of model objects/structs).

Instead, your model objects are often some kind of simpler value type that should be easy to test, and cheap to duplicate en masse. The "reactive bits" of your model should only exist at the layers where changes are being made, and only at a "resolution" that makes sense for the manipulation that is afforded by your UI, or another source of changes. (By "resolution", I mean the difference between having a signal of changing `Person` values, versus a signal of changing `firstName` values, versus a signal for every changing character in the `firstName` string.)

#### Q: Why does `Property.lazyMap` return a `SignalProducer` and not a `Property`?

Doing so eliminates the benefits of evaluating lazily. `Property` requires an initial value from its producer when it is created via `lift` (and asserts when that's not the case), and that behavior doesn't really align with the concept here. We specifically don't want to start a `lazyMap`ped producer up prematurely, and we certainly don't want it spitting out values until its scheduler runs.

So for now, it's just syntactic sugar that prevents you from having to call `lazyMap` on the property's underlying producer.

Includes init(lazyValue:) for SignalProducer, lens for PropertyProtocol, and associated tests to ensure everything works as advertised.

sharplet · 2017-01-26T02:53:52Z

Tests/ReactiveSwiftTests/PropertySpec.swift

+					expect(getterEvaluated).to(beFalse())
+					expect(characters).to(beEmpty())
+
+					testScheduler.run()


I'm planning to respond properly to this, but in the meantime I spotted a theoretical false positive in this test. What do you think about this change? 840bf6a

The test I added merely intended to demonstrate the following:

Before the signal is started, the getter has not been evaluated

When the signal is started, but the getter's queue hasn't scheduled the getter for execution, the getter has still not been evaluated

Once the getter's queue is scheduled, the setter will have run, and the value will have been delivered

I'm probably missing the false positive here, but I think the synchronous execution of the scheduler is a benefit for the specific (simple!) test case I had in mind. Namely, "will the getter get scheduled at all, rather than executing immediately when the SignalProducer is started.."

Now, what you're proposing could help write another test case I wasn't feeling well-equipped to write, which is, "the getter should execute on the specific scheduler/thread that is specified".

Let me know what I might have misinterpreted.

"the getter should execute on the specific scheduler/thread that is specified"

Yeah, that's exactly what I was trying to verify. I agree that in this case having a simpler test is better, but I admit I got caught up in trying to solve the problem of verifying that execution occurs on the expected queue/scheduler. 😛

I believe that code does what it says on the tin (and is thread-safe), so if you think it's valuable to include it feel free to use it as inspiration. Some of those details deserve to be factored out into some kind of test helper anyway, so that the test itself can be higher level.

We might as well go whole-hog if we're going to offer the concept of a lens...

liscio · 2017-01-26T14:23:29Z

I have also just now added the writing side as well in the PR, because it was far too tempting.

Using lenses for reading and writing isn't appropriate for all situations, but it's certainly useful when transacting using value types, and you wish to offer a way to set individual properties on a struct. For example:

let exportedImageSize: MutableProperty<CGSize>(.zero)

let width: BindingTarget<CGFloat> = exportedImageSize.lens { return CGSize(width: $1, height: $0.height) }
let height: BindingTarget<CGFloat> = exportedImageSize.lens { return CGSize(width: $0.width, height: $1) }

width <~ widthTextField.reactive.stringValues
height <~ heightTextField.reactive.stringValues

Personally, this would un-complicate a few spots in my own code where I achieve something similar to the above by taking two discrete width/height properties that I'm forced to combine into a size elsewhere. Now the CGSize value is the true source of truth, rather than a composed view of separate width/height properties.

andersio · 2017-01-26T16:34:15Z

Sources/Property.swift

@@ -678,3 +759,73 @@ public final class MutableProperty<Value>: MutablePropertyProtocol {
 		observer.sendCompleted()
 	}
 }
+
+extension MutableProperty {


Perhaps this can wait until ComposableMutablePropertyProtocol in #182 lands. hmm?

It could, yes. But it's not really holding up the behavior right now.

If #182 goes in first, I'll definitely merge & apply to that. If #182 continues to take a while, I'm OK with a separate PR that moves this to that protocol.

mdiep · 2017-01-27T13:46:30Z

Sources/Property.swift

+	/// - returns: A signal producer that returns values obtained using `getter`
+	///            each time this property's value changes.
+	public func lens<U>(getter: @escaping (Value) -> U) -> SignalProducer<U, NoError> {
+		return producer.flatMap(.latest) { model in SignalProducer(lazyValue: { getter(model) }) }


How does this behave differently than:

return producer.flatMap(.latest) { model in SignalProducer(value: getter(model)) }

?

I don't see how lazyValue: makes any difference here. The returned producer will always be started immediately, so these should be equivalent.

@mdiep In the non-scheduled start variation, I don't think there's a difference as you've found.

mdiep · 2017-01-27T13:54:41Z

Sources/SignalProducer.swift

+	///         .start(on: backgroundScheduler)
+	/// }
+	/// ```
+	public init(lazyValue: @escaping () -> Value) {


This could probably be:

public init(_ block: @escaping () -> Value)

You can think of it as a value preserving type conversion.

I'm not sure whether that'd cause problems in practice. I think it's unlikely to cause conflicts.

It would certainly cut down on verbosity. I'll see how it goes.

The only potential conflict would be if the init(value:) variant went away, because then there'd be confusion between () -> Value and Value itself, forcing closures to always get evaluated (if the compiler wasn't going to balk at the similarity.)

I don't think we'd ever remove the label from init(value: ). So I kinda like the idea of a label-less initializer. (We could also add a variant that uses a completion handler, which would be handy.)

mdiep · 2017-01-27T17:08:01Z

Sources/Property.swift

+			return SignalProducer(lazyValue: { getter(model) })
+				.start(on: scheduler)
+		}
+	}


This would should be the same as this?

return producer .observe(on: scheduler) .flatMap(.latest) { model in SignalProducer(value: getter(model) }

@mdiep I don't think that works in quite the same way, but does it make much of a difference?

According to the docs, what you've pasted above would deliver all the events from the inner signal on the specified scheduler. The start(on:) variant would only evaluate the getter on the scheduler. Because there's only one event getting sent, I'm not sure it really matters all that much.

@mdiep I don't think that works in quite the same way, but it passes the tests both ways.

According to the docs, what you've pasted above would deliver all the events from the inner signal on the specified scheduler. The start(on:) variant would only evaluate the getter on the scheduler. Because there's only one event getting sent, I'm not sure it really matters all that much. Thoughts?

The events are delivered to the flatMap on scheduler.

They do behave differently though… because cancellation happens on a different scheduler. Your way is probably better.

mdiep · 2017-01-29T20:20:56Z

Waiting on @liscio's attempt at lazy or lazyMap as discussed in #239.

liscio · 2017-01-29T23:35:41Z

@mdiep I've got them set up locally but things got a little strange when I was trying to build out some tests and things weren't working out as expected. Hopefully it won't be much longer 'til I get my confusion worked out.

mdiep · 2017-01-30T01:37:25Z

No worries! I just wanted to leave a note (1) so that I remembered the status of this PR and (2) to make sure we were one the same page. ☺️

Renamed the operations based on feedback, and eliminated the non-scheduled variations of lazyMap as they offered none of the benefits that lazy evaluation is offering.

liscio · 2017-01-30T16:04:12Z

I added to the anticipated questions above, and made a number of changes to the code in response to feedback.

I also added some additional tests, and cleaned up the ones that were there.

liscio · 2017-01-30T16:12:24Z

Tests/ReactiveSwiftTests/SignalProducerLiftingSpec.swift

+					// propagate only the first and last values
+					targetScheduler.advance()
+					expect(getterCounter) == 2
+					expect(destination) == ["🎃", "👻"]


@andersio this test result that was a little unexpected. I had hoped that advancing the scheduler would only have sent the final "ghost" value, but it's clear now that the binding target's queue has to batch up its incoming setter invocations.

A .flatMap(.latest) equivalent on the setter side (i.e. switching to the newest scheduled setter call) might be worth exploring since it only ever makes sense to accept the last-set value. Effectively, each value event arriving on the bound signal would have to wipe out all the scheduled setter blocks on its queue and replace with the latest one.

(Yes, I know—easier said than done.)

Well, it shouldn't be hard. A SerialDisposable would do the job. In reality though, it probably wouldn't ever do you a favour, unless you really have a high traffic. But then you might want to throttle your signal or look for back pressure operators, I guess?

Typically I've done this by doing the equivalent of throttle(0, on: UIScheduler()).

mdiep · 2017-01-31T14:15:46Z

Sources/Property.swift

+	/// ```
+	/// let personProperty: MutableProperty<Person>
+	/// let nameProducer: SignalProducer<String, NoError>
+	/// nameProducer = personProperty.lazyLens(on: scheduler) { $0.name }


s/lazyLens/lazyMap/

mdiep · 2017-01-31T14:20:47Z

Sources/Property.swift

+extension PropertyProtocol {
+	/// Returns a `SignalProducer` that sends a new value each time this
+	/// property's value changes, using the supplied `transform` to supply the
+	/// value being sent.


This reads like documentation for map, so it could use some cleanup.

Of note:

that sends a new value each time this property's value changes isn't true. The point of the operator is that it doesn't guarantee a 1-to-1 mapping of input to output.

It doesn't communicate what makes this lazy.

It looks like the other instances of lazyMap have better documentation, so you may just need to copy that here.

mdiep · 2017-01-31T14:21:49Z

Sources/Property.swift

+	/// ```
+	/// let personProperty = MutableProperty(Person(firstName: "Steve", lastName: "McQueen"))
+	/// let firstNameBinding = personProperty.bindingTarget {
+	///         return Person(firstName: $1, lastName: $0.lastName)


Can you name these arguments? I think for documentation purposes that'll be a lot clearer.

mdiep · 2017-01-31T14:37:45Z

Sources/Property.swift

+	///	  - setter: A closure that takes a value of type `U` to produce a new
+	///             value for this property
+	///
+	public func bindingTarget<U>(setter: @escaping (Value, U) -> Value) -> BindingTarget<U> {


This is pretty interesting, but it seems like we should consider this in a separate PR. I don't see any relation or dependency between the two? (Other than these were both originally part of your lens concept.)

mdiep · 2017-01-31T14:38:21Z

Sources/Signal.swift

+	///         will not force the transformation to get invoked immediately
+	///			when a new value is sent. Furthermore, subsequent `.value`
+	///			events will not cause `transform` to repeatedly evaluate when
+	///			it has not been scheduled for execution by `scheduler`.


Indentation here is a bit wonky

mdiep · 2017-01-31T14:38:56Z

Sources/Signal.swift

+	///			when a new value is sent. Furthermore, subsequent `.value`
+	///			events will not cause `transform` to repeatedly evaluate when
+	///			it has not been scheduled for execution by `scheduler`.
+	///


Maybe add an important block that points out that there's not a 1-to-1 mapping between inputs and outputs?

mdiep · 2017-01-31T14:40:45Z

Sources/Signal.swift

+	/// - returns: A signal that sends values obtained using `transform` as this 
+	///            signal sends values.
+	public func lazyMap<U>(on scheduler: SchedulerProtocol, transform: @escaping (Value) -> U) -> Signal<U, Error> {
+		return flatMap(.latest) { model in SignalProducer({ transform(model) }).start(on: scheduler) }


Maybe rename model to value?

This line is a little dense, so I'd also break it up onto multiple lines.

mdiep · 2017-01-31T14:41:37Z

I like this direction a lot. ✨

Are you happy with it @liscio?

liscio · 2017-01-31T16:12:07Z

@mdiep Yeah, I think that things are shaping up fairly well with this. I'll have to modify some of my own usage patterns to figure out how to mix and match these ideas nicely, because a "do-everything" lens method may not be possible.

I really did want to wrap up the ViewModel pattern in a way that was more uniform, where we craft the lenses we want in the view model without much fussing about the constructs used underneath to achieve the pattern. But with Swift 3.1 that need will completely go away!

Why? Because now we can do some really interesting things that get us closer to a clean pattern exposing our model object's properties in the viewModel. For example:

extension Reactive where Base: MutableProperty<Album?>  {
    var titleValues: SignalProducer<String, NoError> {
        return base.producer.map { $0?.title ?? "" }
    }

    var albumArtistValues: SignalProducer<String, NoError> {
        return base.producer.map { $0?.albumArtist ?? "" }
    }
}

So now the need to repeat ourselves when "lensing" our model objects goes away in the view model. The "lens" can be defined once on a specialized extension of MutableProperty and then binding targets can be created in a similar way.

I guess this is a really long way of saying that I'm OK with dropping the lens as a function in favour of exposing effective tools for implementing lens as a pattern.

liscio · 2017-01-31T16:42:15Z

@mdiep I addressed your comments, and in addition to pulling bindingTarget (to be opened in a separate PR) I have also pulled lazyMap from PropertyProtocol because it felt incongruent to the rest of the Property API that returned Property instances.

"Show Invisibles" my arse...

mdiep · 2017-02-02T14:18:40Z

The tests don't compile on Linux. So this will need a little love.

liscio · 2017-02-04T15:32:19Z

Oops. I fixed that Linux build issue earlier, but pushed the change over to the bindingTarget branch. Should be all good once this build completes.

NachoSoto · 2017-02-05T00:42:04Z

Sources/SignalProducer.swift

+	/// - parameters:
+	///   - block: A block that supplies a value to be sent by the `Signal` in
+	///            a `value` event.
+	public init(_ block: @escaping () -> Value) {


Any reason not make this an SignalProducer(value: ) overload that uses @autoclosure instead?

How would that differentiate from the existing non-@autoclosure version?

You wouldn't want to always have a closure—that creates the same problem, just in reverse.

@NachoSoto In a nutshell, @autoclosure implies @noescape, and hence we lose out on the ability to defer execution.

No, you can have an @autoclosure @escaping parameter. I guess it would have to have a different parameter name, you're right, but I think I would make the intention more clear.

@NachoSoto Interesting. I wonder, though, whether we'd still have an issue where the compiler wants to forcefully optimize a passed-in value as being evaluated immediately.

For example, consider the case of SignalProducer(value: callAFunction()) versus the SignalProducer({ callAFunction() }) construct. Would the former be turned into an immediately evaluated variant if the braces are omitted, versus being evaluated lazily?

That seems a little subtle/delicate from a usage standpoint… But yeah—having the lazyValue: as I originally built it might be a good tiebreaker?

I think the unnamed parameter best fits with the other unnamed lossless-type-converting inits that we already have.

I'd also really like to add this variant:

public init<Value, Error>(_ block: @escaping ((Result<Value, Error>) -> Void) -> Void)

Adds lazy value evaluation, and lenses

9190196

Includes init(lazyValue:) for SignalProducer, lens for PropertyProtocol, and associated tests to ensure everything works as advertised.

liscio mentioned this pull request Jan 26, 2017

The "Lazy Lens" #239

Closed

sharplet reviewed Jan 26, 2017

View reviewed changes

Add the writing side to lenses

a293ce8

We might as well go whole-hog if we're going to offer the concept of a lens...

andersio reviewed Jan 26, 2017

View reviewed changes

mdiep reviewed Jan 27, 2017

View reviewed changes

andersio added the enhancement label Jan 27, 2017

mdiep reviewed Jan 27, 2017

View reviewed changes

liscio added 2 commits January 27, 2017 16:02

Adding a new test case

af953b3

Making a few changes based on feedback

9c2f6d1

read lens -> lazyMap, write lens -> bindingTarget

c25b03b

Renamed the operations based on feedback, and eliminated the non-scheduled variations of lazyMap as they offered none of the benefits that lazy evaluation is offering.

liscio changed the title ~~Adds lazy value evaluation, and lenses~~ Adding lazyMap and bindingTarget Jan 30, 2017

liscio commented Jan 30, 2017

View reviewed changes

mdiep requested changes Jan 31, 2017

View reviewed changes

liscio added 2 commits January 31, 2017 11:37

Deleting excessive additions, fixing documentation

6e7d249

Removing whitespace change

a925ee9

liscio added 2 commits January 31, 2017 11:44

Fix some whitespace errors

63b2263

Fix another whitespace bug

f4d5e83

"Show Invisibles" my arse...

liscio changed the title ~~Adding lazyMap and bindingTarget~~ Adding lazyMap Jan 31, 2017

mdiep approved these changes Feb 1, 2017

View reviewed changes

Add missing Dispatch import for Linux

61e5fd5

liscio merged commit 1fca272 into master Feb 4, 2017

liscio deleted the cl-lazy-lens branch February 4, 2017 16:15

NachoSoto reviewed Feb 5, 2017

View reviewed changes

liscio mentioned this pull request May 16, 2018

Parallelize work with RAS #638

Closed

Adding lazyMap #240

Adding lazyMap #240

Conversation

liscio commented Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharplet Jan 26, 2017 • edited Loading

Choose a reason for hiding this comment

liscio commented Jan 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdiep commented Jan 29, 2017

liscio commented Jan 29, 2017

mdiep commented Jan 30, 2017

liscio commented Jan 30, 2017

Choose a reason for hiding this comment

andersio Jan 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdiep commented Jan 31, 2017

liscio commented Jan 31, 2017

liscio commented Jan 31, 2017

mdiep commented Feb 2, 2017

liscio commented Feb 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdiep Feb 5, 2017 • edited Loading

Choose a reason for hiding this comment

Adding `lazyMap` #240

Adding `lazyMap` #240

liscio commented Jan 26, 2017 •

edited

Loading

sharplet Jan 26, 2017 •

edited

Loading

andersio Jan 30, 2017 •

edited

Loading

mdiep Feb 5, 2017 •

edited

Loading