Merge pull request #165 from ReactiveCocoa/binding-operator-fix

Fix broken non-optional to optional bindings.
ReactiveCocoa · Dec 18, 2016 · d01a25e · d01a25e
2 parents c9cb48b + 29d6133
commit d01a25e
Show file tree

Hide file tree

Showing 2 changed files with 87 additions and 34 deletions.
diff --git a/Sources/UnidirectionalBinding.swift b/Sources/UnidirectionalBinding.swift
@@ -87,8 +87,8 @@ public func <~
 	// Alter the semantics of `BindingTarget` to not require it to be retained.
 	// 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> {
+	let observer: Observer<Target.Value, NoError>
+	if let target = target as? BindingTarget<Target.Value> {
 		observer = Observer(value: { [setter = target.setter] in setter($0) })
 	} else {
 		observer = Observer(value: { [weak target] in target?.consume($0) })
@@ -136,10 +136,21 @@ public func <~
 	(target: Target, source: Source) -> Disposable?
 	where Target.Value: OptionalProtocol, Source.Value == Target.Value.Wrapped, Source.Error == NoError
 {
-	let newTarget = BindingTarget<Source.Value>(lifetime: target.lifetime) { [weak target] value in
-		target?.consume(Target.Value(reconstructing: value))
+	// Alter the semantics of `BindingTarget` to not require it to be retained.
+	// 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<Target.Value> {
+		observer = Observer(value: { [setter = target.setter] in setter(Target.Value(reconstructing: $0)) })
+	} else {
+		observer = Observer(value: { [weak target] in target?.consume(Target.Value(reconstructing: $0)) })
 	}
-	return newTarget <~ source
+
+	let disposable = source.observe(observer)
+	if let disposable = disposable {
+		target.lifetime.ended.observeCompleted { disposable.dispose() }
+	}
+	return disposable
 }
 
 /// A binding target that can be used with the `<~` operator.

diff --git a/Tests/ReactiveSwiftTests/UnidirectionalBindingSpec.swift b/Tests/ReactiveSwiftTests/UnidirectionalBindingSpec.swift
@@ -11,49 +11,91 @@ class UnidirectionalBindingSpec: QuickSpec {
 			var token: Lifetime.Token!
 			var lifetime: Lifetime!
 			var target: BindingTarget<Int>!
-			var value: Int!
+			var optionalTarget: BindingTarget<Int?>!
+			var value: Int?
 
 			beforeEach {
 				token = Lifetime.Token()
 				lifetime = Lifetime(token)
 				target = BindingTarget(lifetime: lifetime, setter: { value = $0 })
+				optionalTarget = BindingTarget(lifetime: lifetime, setter: { value = $0 })
 				value = nil
 			}
 
-			it("should pass through the lifetime") {
-				expect(target.lifetime).to(beIdenticalTo(lifetime))
-			}
-
-			it("should stay bound after deallocation") {
-				weak var weakTarget = target
-
-				let property = MutableProperty(1)
-				target <~ property
-				expect(value) == 1
-
-				target = nil
-
-				property.value = 2
-				expect(value) == 2
-				expect(weakTarget).to(beNil())
-			}
+			describe("non-optional target") {
+				it("should pass through the lifetime") {
+					expect(target.lifetime).to(beIdenticalTo(lifetime))
+				}
 
-			it("should trigger the supplied setter") {
-				expect(value).to(beNil())
+				it("should stay bound after deallocation") {
+					weak var weakTarget = target
 
-				target.consume(1)
-				expect(value) == 1
+					let property = MutableProperty(1)
+					target <~ property
+					expect(value) == 1
+
+					target = nil
+
+					property.value = 2
+					expect(value) == 2
+					expect(weakTarget).to(beNil())
+				}
+
+				it("should trigger the supplied setter") {
+					expect(value).to(beNil())
+
+					target.consume(1)
+					expect(value) == 1
+				}
+
+				it("should accept bindings from properties") {
+					expect(value).to(beNil())
+
+					let property = MutableProperty(1)
+					target <~ property
+					expect(value) == 1
+
+					property.value = 2
+					expect(value) == 2
+				}
 			}
 
-			it("should accept bindings from properties") {
-				expect(value).to(beNil())
+			describe("optional target") {
+				it("should pass through the lifetime") {
+					expect(optionalTarget.lifetime).to(beIdenticalTo(lifetime))
+				}
 
-				let property = MutableProperty(1)
-				target <~ property
-				expect(value) == 1
+				it("should stay bound after deallocation") {
+					weak var weakTarget = optionalTarget
 
-				property.value = 2
-				expect(value) == 2
+					let property = MutableProperty(1)
+					optionalTarget <~ property
+					expect(value) == 1
+
+					optionalTarget = nil
+
+					property.value = 2
+					expect(value) == 2
+					expect(weakTarget).to(beNil())
+				}
+
+				it("should trigger the supplied setter") {
+					expect(value).to(beNil())
+
+					optionalTarget.consume(1)
+					expect(value) == 1
+				}
+
+				it("should accept bindings from properties") {
+					expect(value).to(beNil())
+
+					let property = MutableProperty(1)
+					optionalTarget <~ property
+					expect(value) == 1
+
+					property.value = 2
+					expect(value) == 2
+				}
 			}
 
 			it("should not deadlock on the same queue") {