ReactiveCocoa / ReactiveSwift

Streams of values over time
MIT License
3k stars 432 forks source link

Kill Actions as BindingTargets? #54

Closed liscio closed 8 years ago

liscio commented 8 years ago

@andersio and I (with some @sharplet thrown in) had a discussion recently about the ability to bind to an Action in ReactiveSwift.

My distaste for this feature started when I saw the following code in a test case:

presented.rac.dismissAnimated <~ Property(value: false)

Frankly I don't feel that it's a good idea to enable this sort of use through the API, and I think that the ability to bind an Action to an arbitrary signal seems like a solution looking for a problem.

I'm not against the pattern of an <~ operator showing up in the UI code, but rather I think that it should be limited to binding only "action outlets" to "action inlets", so to speak. To paraphrase a comment from @andersio: "it should work more like IB".

It's possible that I'm in need of some education and examples of where this is useful in practice. I don't use actions all that much in practice outside of the UI case, where controls are being bridged to actions. And in all these cases, I think that my uses may be a bit of a stretch.

So, I say we either pull it, or come with some compelling real-world cases outside of UI code where this holds value.

mdiep commented 8 years ago

So, I say we either pull it, or come with some compelling real-world cases outside of UI code where this holds value.

TBH, I'm not sure how much value Action itself has outside of UI code. It's primary benefits, like serialization, are mostly important in a long-lived process.

But I'll try to think of an example where it'd be useful.

liscio commented 8 years ago

@mdiep one that I came up with myself was a "bridge to the imperative world" similar to the following:

final class SomeDatabaseThingy {
    // Something that performs a flush of the underlying database on a successful fetch
    let flushAction: Action<Bool, Void, NoError> 

    let fetchOperation: SignalProducer<SomeValue?, NoError>

    func doAFetch() {
        fetchOperation.startWithSignal { signal, _ in
            // Something that stores the fetchOperation result in the database
            someProperty <~ signal
            flushAction <~ signal.map { $0 != nil }
        }
    }
}

The alternative, of course, would be to use .on(next:) to call the database's flush operation. This is likely not a great example of usage, but it was all I could come up with. :)

liscio commented 8 years ago

I think that it makes more sense to simply discourage things like dismissAnimated() being wrapped in actions rather than taking away this potentially useful feature.

I'm closing this one out so we don't have to waste any more cycles on it…