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

Changes to the DynamicProperty semantics. #3162

Closed
wants to merge 5 commits into from

Conversation

andersio
Copy link
Member

@andersio andersio commented Aug 31, 2016

Make it more Property alike - a lens to a key path of an NSObject without its own lifetime.

  1. A DynamicProperty retains its lensing object.

  2. The producer and signal respects the lifetime of the object being lensed.

  3. Bindings targeting DynamicProperty ties directly to the object being lensed.

  4. DynamicProperty is now leak safe - reusing instances is no longer necessary.

    For example:

    DynamicProperty<Int>(object: controller,
                         keyPath: #keyPath(Controller.value)) <~ aProducer
    

    would still be valid even if the DynamicProperty has gone out of scope. It would last until controller deinitializes, or aProducer terminates.

@NachoSoto
Copy link
Member

NachoSoto commented Sep 2, 2016

A DynamicProperty retains its lensing object.

👎 I don't think this is the way to go. A DynamicProperty represents the values over time for a given object's property, which does not mean that it //owns// the object. In fact, it means the complete opposite. This is equivalent to how NSNotificationCenter does not retain the object (#2859, for example).

The producer and signal respects the lifetime of the object being lensed.

Now, //these// semantics I think are correct. But the previous point kind of breaks this because there would be one extra artificial owner.

@@ -5,71 +5,70 @@ import enum Result.NoError
/// types, including generic types when boxed via `AnyObject`).
private protocol ObjectiveCRepresentable {
associatedtype Value
static func extract(from representation: Any) -> Value
static func represent(_ value: Value) -> Any
static func extract(from representation: Any?) -> Value
Copy link
Member

Choose a reason for hiding this comment

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

Why Optional? representation can always be an optional (since it's Any).

Copy link
Member Author

@andersio andersio Sep 3, 2016

Choose a reason for hiding this comment

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

If you pass an optional to Any, it would be bridged to an opaque box (SwiftValue) regardless of being nil or not. The KVC interface in fact anticipates Any?.

The way DynamicProperty is initialised also restricted the implementation options. Since optionals are neither bridgeable nor conforming to AnyObject, new initializers have to be tailored for optionals with the OptionalProtocol constraints. Unless we assume all key paths are nullable & requires manual nil skipping.

@andersio
Copy link
Member Author

andersio commented Sep 3, 2016

It might be confusing since DynamicProperty was (IIRC) originally introduced to serve as a binding mediator, and hence wasn't "owning" the object but in another way round to avoid reference cycles. But with the introduction of BindingTarget, it is now possible to establish bindings without such mediator. So I proposed to reposition it as a "lens".

It creates a reduced view (lens) to the NSObject, which concerns only a certain key path. In this sense, you are still accessing the object directly, i.e. conceptually the same as let value = { object.property }.

However, all other stuff (producer/signal/lifetime/binding) is decoupled and instead derived directly from the underlying object. This aligns with our property composition and signal lifetime semantics.

For example, if you retain propertyA.map { $0.name } as name, propertyA is retained by name even if you throw your last reference to it away.

Now treat propertyA as the NSObject, and name as the DynamicProperty. :)

It also eliminates potential leaks due to misuse, since at this moment every instance of DynamicProperty is automatically retained by the observed object upon creation.

This is actually similar to Groff's idea of "property lenses" for property reflection, say a () -> inout U, where a full property interface (that captures the object/struct) can be obtained and passed around.

@mdiep
Copy link
Contributor

mdiep commented Sep 10, 2016

I agree with @NachoSoto on this.

I think the weak semantics are more common here; although you'll certainly want strong on occasion. But it's also easier to make the semantics stronger than to make them weaker when you want the other behavior.

I definitely get what you're saying about the lens behavior. But if we want to support those semantics, they need to be in addition to the current semantics—not as a replacement.

@mdiep
Copy link
Contributor

mdiep commented Oct 1, 2016

I don't think we should give DynamicProperty lens semantics. I don't think that's the role it plays. It's meant to make values reactive/observable. It's similar to a lens, but I think going down that route is a mistake that distracts from the core value of RAC.

Going back to your original example:

DynamicProperty<Int>(object: controller,
                     keyPath: #keyPath(Controller.value)) <~ aProducer

I don't think that provides much value. If the DynamicProperty isn't kept around to reuse or observe, then there's no point in using a property. You can just as easily use startWithValues.

@andersio
Copy link
Member Author

andersio commented Oct 1, 2016

Yeah, and that lens-ish part has been removed. What's left here is the removal of the strong reference, so that DynamicProperty needn't be deliberately uniqued.

@mdiep
Copy link
Contributor

mdiep commented Oct 1, 2016

Yeah, and I just don't think those are the right semantics for DynamicProperty. 😞

@andersio
Copy link
Member Author

andersio commented Oct 1, 2016

I don't think the semantics have changed per the state of PR, other than lifting the need of uniquing. If we agree that the main rationale of DynamicProperty being bound to the underlying object is to allow the bindings to propagate values, the PR just does the same in an alternative way.

If the need of uniquing is a deliberate feature, I would argue that the initializer should hide behind something that does the uniquing, instead of the slippery ground we have now.

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

3 participants