-
Notifications
You must be signed in to change notification settings - Fork 429
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
BindingSourceProtocol #131
Conversation
/// - returns: A disposable that can be used to terminate binding before the | ||
/// deinitialization of the target or the signal's `completed` | ||
/// event. | ||
associatedtype Error: Swift.Error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would need an error type until Swift 4 since we can't add conditional conformance when Error == NoError
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
} | ||
|
||
@discardableResult | ||
public func <~ <Target: BindingTargetProtocol, Source: BindingSourceProtocol>(target: Target, source: Source) -> Disposable? where Source.Value == Target.Value, Source.Error == NoError { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be defined in an extension to BindingTargetProtocol
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it's generic over both sides, I think it makes sense to define it at the top level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me!
Also, total nitpick, but I think this diff might be a little easier to read if BindingTargetProtocol
appeared first in the file.
associatedtype Value | ||
|
||
/// The lifetime of `self`. The binding operators use this to determine when | ||
/// the binding should be teared down. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should read "torn down"
@discardableResult | ||
static func <~ <Source: SignalProtocol>(target: Self, signal: Source) -> Disposable? where Source.Value == Value, Source.Error == NoError | ||
func observe(_ observer: Observer<Value, Error>, lifetime: Lifetime) -> Disposable? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why an optional Disposable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signal.observe
returns an optional.
/// Describes a target to which can be bound. | ||
public protocol BindingTargetProtocol: class { | ||
/// Describes a source which can be bound. | ||
public protocol BindingSourceProtocol { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it be a class
bound protocol?
Edit: Unless we might conform collection types to BindingSourceProtocol
too, and implicitly lift them as producers, this should be a class bound protocol.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SignalProducer
isn't a class, which is why this isn't class bound.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, overlooked that. 🙈
func consume(_ value: Value) | ||
} | ||
|
||
/// Binds a signal to a target, updating the target's value to the latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the documentation.
@discardableResult | ||
public static func <~ <Source: PropertyProtocol>(target: Self, property: Source) -> Disposable where Source.Value == Value.Wrapped { | ||
return target <~ property.producer | ||
/// Binds a source to a target, updating the target's value to the latest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto.
/// event. | ||
associatedtype Error: Swift.Error | ||
|
||
/// Observe the binding source by sending any evenst to the given observer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo: evenst
.
/// - returns: An optional `Disposable` which can be used to disconnect the | ||
/// observer. | ||
/// - returns: A `Disposable` which can be used to disconnect the observer, | ||
/// or `nil` if the signal has already completed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Nit] "already terminated" sounds more accurate.
.observeValues { [setter = target.setter] value in | ||
setter(value) | ||
} | ||
} | ||
} | ||
|
||
private let specificKey = DispatchSpecificKey<ObjectIdentifier>() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems unused.
return signal | ||
.take(during: target.lifetime) | ||
.observeValues { [setter = target.setter] value in | ||
setter(value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be removed. It is intentionally overridden to capture setter
directly, so that self
need not be retained and/or uniqued by its parent. The default implementation weakly captures the target, so the binding would be invalid if the target is not retained.
(Seems like it wasn't covered in the spec.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a test for this in 1c33980.
// This is done here--and not in a separate function--so that all variants | ||
// of `<~` can get this behavior. | ||
let observer: Observer<Source.Value, NoError> | ||
if let target = target as? BindingTarget<Source.Value> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IIRC Swift would always pick the overload with the most specific matching constraint. So instead of a dynamic check, you might as well define:
@discardableResult
public func <~
<Value, Source: BindingSourceProtocol>
(target: BindingTarget<Value>, source: Source) -> Disposable?
where Source.Value == Value, Source.Error == NoError
Though the alternative is still having it defined in the protocol, and overriding it in BindingTarget
.
(Not sure how it's going to behave if one is global while another is in the type.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue I'm nervous about is if I want to constrain the righthand side to be something else.
public func <~
<Target: BindingTargetProtocol>
(target: Target, source: FooSource) -> Disposable?
where Target.Value == FooSource.Value
{
… // This would call to another <~
}
But maybe that'd need to filter through the overload anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure about the behavior. But there seems no point to assume the need of custom binding logic for varying types of sources.
Though I believe if you define it in the protocol, the matching priority would likely be given to the target.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer the solution as-is because I know exactly how it will behave.
The effect is the same. So why not use the solution that we understand and know will work?
The idea here is to eliminate redundancy in things like ReactiveCocoa/ReactiveCocoa#3289.
By adding a
BindingSourceProtocol
type, both ends of<~
become generic. That makes it easier to add conformance on either end.This could also be called
Observable
—if we want to make it some more generic.I'm curious what people think of this approach.