-
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
[api breaking]Make some operators to accept SignalProducerConvertible conforming types #611
[api breaking]Make some operators to accept SignalProducerConvertible conforming types #611
Conversation
…rror, Signal.flatMapError are now accept SignalProducerConvertible conforming type
…onvertible conforming type
Sources/SignalProducer.swift
Outdated
@@ -2143,7 +2143,7 @@ extension SignalProducer { | |||
/// | |||
/// - returns: A producer that sends events from `self` and then from | |||
/// `replacement` when `self` completes. | |||
public func then<U>(_ replacement: SignalProducer<U, Error>) -> SignalProducer<U, Error> { | |||
public func then<U, Replacement: SignalProducerConvertible>(_ replacement: Replacement) -> SignalProducer<U, Error> where Replacement.Value == U, Replacement.Error == 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.
U
can be replaced by Replacement.Value
.
Sources/SignalProducer.swift
Outdated
return _then(replacement) | ||
} | ||
|
||
// NOTE: The method below is the shared implementation of `then(_:)`. The underscore | ||
// prefix is added to avoid self referencing in `then(_:)` overloads with | ||
// regard to the most specific rule of overload selection in Swift. | ||
|
||
internal func _then<U>(_ replacement: SignalProducer<U, Error>) -> SignalProducer<U, Error> { | ||
internal func _then<U, Replacement: SignalProducerConvertible>(_ replacement: Replacement) -> SignalProducer<U, Error> where Replacement.Value == U, Replacement.Error == 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.
Ditto.
Sources/SignalProducer.swift
Outdated
@@ -2203,7 +2203,7 @@ extension SignalProducer where Error == NoError { | |||
/// | |||
/// - returns: A producer that sends events from `self` and then from | |||
/// `replacement` when `self` completes. | |||
public func then<U, NewError>(_ replacement: SignalProducer<U, NewError>) -> SignalProducer<U, NewError> { | |||
public func then<U, NewError, Replacement: SignalProducerConvertible>(_ replacement: Replacement) -> SignalProducer<U, NewError> where Replacement.Value == U, Replacement.Error == NewError { |
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. NewError
can be replaced by Replacement.Error
too.
/// | ||
/// - returns: A producer that sends events from `self` and then from | ||
/// `replacement` when `self` completes. | ||
public func then<Replacement: SignalProducerConvertible>(_ replacement: Replacement) -> SignalProducer<Value, NoError> where Replacement.Value == Value, Replacement.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.
Why is there a new variant?
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.
Probably because the type became ambiguous than before.
In the test, 4 errors occur including this line.
Sources/Flatten.swift
Outdated
@@ -960,7 +963,7 @@ extension SignalProducer { | |||
/// - parameters: | |||
/// - transform: A closure that accepts emitted error and returns a signal | |||
/// producer with a different type of error. | |||
public func flatMapError<F>(_ transform: @escaping (Error) -> SignalProducer<Value, F>) -> SignalProducer<Value, F> { | |||
public func flatMapError<F, Inner: SignalProducerConvertible>(_ transform: @escaping (Error) -> Inner) -> SignalProducer<Value, F> where Inner.Value == Value, Inner.Error == F { |
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.
F
can be replaced by Inner.Error
.
Sources/Flatten.swift
Outdated
@@ -928,19 +928,22 @@ extension Signal { | |||
/// - parameters: | |||
/// - transform: A closure that accepts emitted error and returns a signal | |||
/// producer with a different type of error. | |||
public func flatMapError<F>(_ transform: @escaping (Error) -> SignalProducer<Value, F>) -> Signal<Value, F> { | |||
public func flatMapError<F, Inner: SignalProducerConvertible>(_ transform: @escaping (Error) -> Inner) -> Signal<Value, F> where Inner.Value == Value, Inner.Error == F { |
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.
Sources/Flatten.swift
Outdated
return Signal<Value, F> { observer, lifetime in | ||
lifetime += self.observeFlatMapError(transform, observer, SerialDisposable()) | ||
} | ||
} | ||
|
||
fileprivate func observeFlatMapError<F>(_ handler: @escaping (Error) -> SignalProducer<Value, F>, _ observer: Signal<Value, F>.Observer, _ serialDisposable: SerialDisposable) -> 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.
Ditto.
CHANGELOG.md
Outdated
@@ -5,7 +5,7 @@ | |||
1. New property operator: `filter` (#586, kudos to @iv-mexx) | |||
1. New operator `merge(with:)` (#600, kudos to @ra1028) | |||
1. New operator `map(value:)` (#601, kudos to @ra1028) | |||
1. `SignalProducer.merge` now accepts any combination of `SignalProducerConvertible` conforming types (#610, kudos to @1028) | |||
1. `SignalProducer.merge`, `SignalProducer.concat`, `SignalProducer.prefix`, `SignalProducer.then`, `SignalProducer.flatMapError` `Signal.flatMapError`, `Signal.withLatest` and `Property.init(initial:values:)` are now accept `SignalProducerConvertible` conforming types (#610, #611, kudos to @1028) |
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.
Property.init(initial:then:)
.
@andersio |
@@ -3012,7 +3012,7 @@ class SignalProducerSpec: QuickSpec { | |||
.combineLatest(SignalProducer<Int, NoError>.never.promoteError(), | |||
SignalProducer<Double, TestError>.never, | |||
SignalProducer<Float, NoError>.never.promoteError(), | |||
SignalProducer<UInt, POSIXError>.never.flatMapError { _ in .empty }) | |||
SignalProducer<UInt, POSIXError>.never.flatMapError { _ in SignalProducer.empty }) |
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'd say this is a regression. I use this very often, so it's unfortunate it doesn't compile anymore... I wonder if adding static var empty
to the protocol would still allow this?
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.
Do you mean adding static var empty
to the SignalProducerConvertible
?
I think it can't be resolved without a contextual types😢
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.
@NachoSoto
One of solution is to overload SignalProducer.flatMapError
as below.
But I don't prefer this.
public func flatMapError<Inner: SignalProducerConvertible>(_ transform: @escaping (Error) -> Inner) -> SignalProducer<Value, Inner.Error> where Inner.Value == Value {
return flatMapError { transform($0).producer }
}
public func flatMapError<NewError>(_ transform: @escaping (Error) -> SignalProducer<Value, NewError>) -> SignalProducer<Value, NewError> {
return SignalProducer<Value, NewError> { observer, lifetime in
let serialDisposable = SerialDisposable()
lifetime += serialDisposable
self.startWithSignal { signal, signalDisposable in
serialDisposable.inner = signalDisposable
_ = signal.observeFlatMapError(transform, observer, serialDisposable)
}
}
}
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.
@ra1028 I think it is a reasonable solution & should be applied to existing operators anticipating SignalProducerConvertible
too. Contextual lookup plays an important role in expressivity.
That's said, please try if a default implementation of static var empty: SignalProducer<Value, Error>
on the protocol works.
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.
@andersio
Such protocol works to occur errors like the below.
extension SignalProducerConvertible {
/// A producer for a Signal that immediately completes without sending any values.
public static var empty: SignalProducer<Value, Error> {
return .empty
}
}
Therefore, I'll overload existing operators anticipating SignalProducerConvertible
.
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 an overloaded operator to flatMap
and flatMapError
for contextual lookup.
Is it also necessary for other operators that will passed SignalProducerConvertible
?
IMO, there aren't many cases to pass empty
directly like below.
producer.then(.empty)
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.
It is not just .empty
but also .init
and generic expressions which is handful when contextual lookup & type inference work. So yes.
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.
@andersio
done 👍
CHANGELOG.md
Outdated
@@ -5,7 +5,7 @@ | |||
1. New property operator: `filter` (#586, kudos to @iv-mexx) | |||
1. New operator `merge(with:)` (#600, kudos to @ra1028) | |||
1. New operator `map(value:)` (#601, kudos to @ra1028) | |||
1. `SignalProducer.merge` now accepts any combination of `SignalProducerConvertible` conforming types (#610, kudos to @1028) | |||
1. `SignalProducer.merge`, `SignalProducer.concat`, `SignalProducer.prefix`, `SignalProducer.then`, `SignalProducer.flatMapError` `Signal.flatMapError`, `Signal.withLatest` and `Property.init(initial:then:)` are now accept `SignalProducerConvertible` conforming types (#610, #611, kudos to @1028) |
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.
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.
@mdiep
Sorry for the late response.
Done:+1:
/// - returns: A producer that will start `self` and then on completion of | ||
/// `self` - will start `next`. | ||
public func concat<Next: SignalProducerConvertible>(_ next: Next) -> SignalProducer<Value, Error> where Next.Value == Value, Next.Error == Error { | ||
return concat(next.producer) | ||
} |
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 do we need 2 versions of all these operators?
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.
To disambiguate when contextual lookup is used with these operators. Context: #611 (comment)
Ideally there should be a way of saying Next
by default would be SignalProducer
, for example. But there wasn't so the only option is relying on overloading.
I think we'd want tests that exercise each of these at least once—for type inference if nothing else. I'm going go to leave this for @andersio to review since he'll be more much familiar with any traps here. |
…lacement:), sample(with:), sample(on:), withLatest(from:), combineLatest(with:) and zip(with:)
182bda5
to
f5ca560
Compare
Continuation from #610
These operators now accept
SignalProducerConvertible
conforming types.SignalProducer.merge(with:)
SignalProducer.concat
SignalProducer.prefix
SignalProducer.then
SignalProducer.and
SignalProducer.or
SignalProducer.zip(with:)
SignalProducer.sample(with:)
SignalProducer.sample(on:)
SignalProducer.take(until:)
SignalProducer.take(untilReplacement:)
SignalProducer.skip(until:)
SignalProducer.flatMap
SignalProducer.flatMapError
SignalProducer.combineLatest(with:)
Signal.flatMap
Signal.flatMapError
Signal.withLatest(from:)
Property.init(initial:then:)
Checklist