Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add binding operator to Signal.Observer #635

Merged

Conversation

Marcocanc
Copy link
Member

@Marcocanc Marcocanc commented Apr 25, 2018

As the title says, I think it would be nice to make Observer conform to BindingTargetProvider.
Any thoughts on that?

This PR adds the binding operator to Signal.Observer

Checklist

  • Updated CHANGELOG.md.

Copy link
Contributor

@sharplet sharplet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a fan, I've wanted this myself recently 👍

@@ -124,6 +125,12 @@ extension Signal {
}
}

extension Signal.Observer: BindingTargetProvider {
public var bindingTarget: BindingTarget<Value> {
return BindingTarget(lifetime: lifetime, action: send)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is implicitly making a strong reference to self here. Might need a weak reference to ensure the binding doesn't keep the observer alive?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not quite convinced, because usually there is an owner whose lifetime the binding target can use.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you saying another lifetime should be passed in when initialising the observer?

Copy link
Member

@andersio andersio Apr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Input Observers have to be owned by someone, and IMO it should be that someone who uses its own lifetime to create the binding target for the pipe.

We could have a convenience that takes a lifetime to create a binding target though.

Copy link
Member

@andersio andersio May 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This strongly captures self by the way, which goes against the contract of <~.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted the commit that fixes that in order to un-hide the discussion that was going on. Will change it again if/when we decide how to proceed with this PR.

@mdiep
Copy link
Contributor

mdiep commented Apr 27, 2018

Makes sense. 👍

Can you add some tests and a changelog entry?

@Marcocanc
Copy link
Member Author

Marcocanc commented Apr 27, 2018

@mdiep Will do, but there's still an ongoing discussion about which lifetime to use. (Unfortunately I buried it by making another commit) I removed the commit for the sake of visibility :)

@Marcocanc Marcocanc force-pushed the feature/observable-bindingtarget branch from 0b72df0 to 75470fe Compare April 27, 2018 12:36
@andersio
Copy link
Member

andersio commented May 7, 2018

The (premature?) optimisation side of me has a little problem with this adding 16B overhead plus (at least) two objects per Observer. This is a convenience I’d be hestitate to make, especially in the light of the idea that Observer should not have its own lifetime. Observer is destined to be owned by someone that has a “primary” lifetime — either a Signal for receiving events, or the sender for pushing out events.

I would argue a majority of cases can already be satisfied by start* or observe* plus a strong Observer-captured method reference. This is more explicit in the sense of capturing, and on the contrary, <~ is meant to weakly reference both ends.

If this convenience is strongly desired, my proposed alternative would be a special overload for self.reactive[\.myObserver] <~ myProducer, which has all the information needed to construct a binding that doesn’t violate any assumption made above, and doesn’t carry the overhead introduced by this branch. Though the downside is this being exclusive to ReactiveExtensionsProvider until we can extend AnyObject (maybe never).

@@ -124,6 +125,12 @@ extension Signal {
}
}

extension Signal.Observer: BindingTargetProvider {
public var bindingTarget: BindingTarget<Value> {
return BindingTarget(lifetime: lifetime, action: send)
Copy link
Member

@andersio andersio May 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This strongly captures self by the way, which goes against the contract of <~.

@Marcocanc
Copy link
Member Author

I had two main motivations for this PR:

  1. When binding ViewModels that use Observers as inputs it's not very nice to have two ways of binding (<~ operator for outputs and observe* for inputs). With the binding operator the target will be on the left, whereas with observe it will be on the right.

  2. I use MVVM for UICollectionViewCells extensively. In that case I want the observer to only receive values and no terminating events (otherwise the ViewModel's pipe would break whenever the cell is reused since the binding is disposed)

@andersio Here's another approach:

extension Signal.Observer {
    public static func <~
        <Source: BindingSource>
        (observer: Signal<Value, Error>.Observer, source: Source) -> Disposable?
        where Source.Value == Value, Source.Error == NoError
    {
        return source.producer.start { [weak observer] in
            switch $0 {
            case .value(let val):
                observer?.send(value: val)
            default: break
            }
        }
    }
}

@andersio
Copy link
Member

andersio commented May 27, 2018

@Marcocanc I think the approach in #635 (comment) is fair. :)

@Marcocanc Marcocanc force-pushed the feature/observable-bindingtarget branch from 42695c4 to 60ab0ac Compare January 22, 2019 19:27
@Marcocanc Marcocanc changed the title Make Observer conform to BindingTargetProvider Add binding operator to Signal.Observer Jan 22, 2019
@Marcocanc
Copy link
Member Author

@andersio I have updated the PR with the proposed approach 😊 Sorry it took me so long

@mdiep mdiep requested a review from andersio January 30, 2019 13:44
Copy link
Contributor

@mdiep mdiep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a conflict in the CHANGELOG now. Let's get that fixed and then we can merge. 👍

Sources/UnidirectionalBinding.swift Outdated Show resolved Hide resolved
@Marcocanc Marcocanc force-pushed the feature/observable-bindingtarget branch from ee5026d to f1cb6f3 Compare May 22, 2019 12:45
@mdiep mdiep merged commit ed5f02e into ReactiveCocoa:master May 22, 2019
@mdiep
Copy link
Contributor

mdiep commented May 22, 2019

Thanks for the PR! Sorry this took so long. 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants