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

Property composition operators. #2922

Merged
merged 19 commits into from Jun 5, 2016

Conversation

Projects
None yet
2 participants
@andersio
Member

andersio commented May 22, 2016

A subset of #2911.

Property Composition
  1. New combining operators: combineLatest and zip. Also their top-level binary to 10-ary shorthands.
  2. New value operators: combinePrevious, skipRepeats and uniqueValues
  3. New flattening operators: flatten and flatMap.

The signals and producers of AnyPropertys respect the lifetime of its ultimate source(s). In other words:

someIntProperty.map { $0 + 1 }.map { $0 + 2 }.map { $0 + 3 }.producer

would be alive as long as someIntProperty is still alive, even if all the intermediate AnyPropertys are deinitialised.

Breaking change

The producer and the signal of the resulting property of map has now a different lifetime semantics. That's said map is not yet included in a release.

Miscellaneous
  1. Reorganised the Property.swift, with protocols & protocol extensions at the top, followed by implementations, bindings and finally the private utility classes.
@andersio

This comment has been minimized.

Show comment
Hide comment
@andersio

andersio May 22, 2016

Member

This branch includes a change to the lifetime of the producers and signals of the transformed properties, as suggested by @mdiep. They now respect the lifetime of the ultimate source, instead of the internal relaying property.

Moreover, intermediate properties in a chain would not be retained but only the ultimate source, so that their internal relaying MutablePropertys can be released.

For example, property.map { $0 }.map { $0 }.map { $0 }.producer is now alive as long as property's lifetime. Moreover, it would no longer result in 3 MutablePropertys being unnecessarily kept alive just for propagating the changes.

It is likely a better solution to the unsafe operator chaining issue once brought up by @filblue (#2788 (comment)).

Member

andersio commented May 22, 2016

This branch includes a change to the lifetime of the producers and signals of the transformed properties, as suggested by @mdiep. They now respect the lifetime of the ultimate source, instead of the internal relaying property.

Moreover, intermediate properties in a chain would not be retained but only the ultimate source, so that their internal relaying MutablePropertys can be released.

For example, property.map { $0 }.map { $0 }.map { $0 }.producer is now alive as long as property's lifetime. Moreover, it would no longer result in 3 MutablePropertys being unnecessarily kept alive just for propagating the changes.

It is likely a better solution to the unsafe operator chaining issue once brought up by @filblue (#2788 (comment)).

@andersio andersio closed this May 23, 2016

@andersio andersio reopened this May 23, 2016

@andersio andersio closed this May 23, 2016

@andersio andersio reopened this May 23, 2016

Show outdated Hide outdated ReactiveCocoa/Swift/Property.swift Outdated
Show outdated Hide outdated ReactiveCocoa/Swift/Property.swift Outdated
Show outdated Hide outdated ReactiveCocoa/Swift/Property.swift Outdated
@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep May 24, 2016

Contributor

AnyProperty now uses a private, type erasing box.

What's the motivation for this?

A new type erased wrapper AnyMutableProperty.

What's the motivation for this? Can we break it out into a separate PR?

It now targets the RAC5 branch since it contains breaking changes

What are the breaking changes? The changes to property lifetimes?

Contributor

mdiep commented May 24, 2016

AnyProperty now uses a private, type erasing box.

What's the motivation for this?

A new type erased wrapper AnyMutableProperty.

What's the motivation for this? Can we break it out into a separate PR?

It now targets the RAC5 branch since it contains breaking changes

What are the breaking changes? The changes to property lifetimes?

@andersio

This comment has been minimized.

Show comment
Hide comment
@andersio

andersio May 25, 2016

Member

AnyProperty now uses a private, type erasing box.

What's the motivation for this?

To allow AnyProperty to be able to pass through withValue of its wrapping property. By so, the consumer of an AnyProperty wrapping a atomic property like MutableProperty can do stuff atomically with regard to the wrapped property's lock if necessary.

let mutableProperty = MutableProperty(1)
let readOnlyProperty = AnyProperty(mutableProperty)
readOnlyProperty.withValue { value in
    /// The execution of this closure is protected by `mutableProperty`'s recursive lock.
}

This is impossible to express in Swift 2.x using the current closure approach due to the limitation of @noescape, while in Swift 3 it (so far) still lacks the ability to express a rethrows function type.

Using a box also reduces the number of retains when passing/copying an AnyProperty on paper, though the real-world difference is likely negligible.

A new type erased wrapper AnyMutableProperty.

What's the motivation for this? Can we break it out into a separate PR?

Sure. For the motivation, as written in the preceding PR:

AnyMutableProperty is introduced for users who would like to build higher level constructs that operate on the MutablePropertyType abstraction. An example would be two-way bindings between two MutablePropertyTypes without knowing the concrete type.

It now targets the RAC5 branch since it contains breaking changes

What are the breaking changes? The changes to property lifetimes?

  1. New protocol requirements of PropertyType and MutablePropertyType.
  2. The producer/signal lifetime of AnyProperty created using init(initialValue:signal:) or init(initialValue:producer:).

The lifetime of transformed property - currently only map(transform:) - is unaffected.

Member

andersio commented May 25, 2016

AnyProperty now uses a private, type erasing box.

What's the motivation for this?

To allow AnyProperty to be able to pass through withValue of its wrapping property. By so, the consumer of an AnyProperty wrapping a atomic property like MutableProperty can do stuff atomically with regard to the wrapped property's lock if necessary.

let mutableProperty = MutableProperty(1)
let readOnlyProperty = AnyProperty(mutableProperty)
readOnlyProperty.withValue { value in
    /// The execution of this closure is protected by `mutableProperty`'s recursive lock.
}

This is impossible to express in Swift 2.x using the current closure approach due to the limitation of @noescape, while in Swift 3 it (so far) still lacks the ability to express a rethrows function type.

Using a box also reduces the number of retains when passing/copying an AnyProperty on paper, though the real-world difference is likely negligible.

A new type erased wrapper AnyMutableProperty.

What's the motivation for this? Can we break it out into a separate PR?

Sure. For the motivation, as written in the preceding PR:

AnyMutableProperty is introduced for users who would like to build higher level constructs that operate on the MutablePropertyType abstraction. An example would be two-way bindings between two MutablePropertyTypes without knowing the concrete type.

It now targets the RAC5 branch since it contains breaking changes

What are the breaking changes? The changes to property lifetimes?

  1. New protocol requirements of PropertyType and MutablePropertyType.
  2. The producer/signal lifetime of AnyProperty created using init(initialValue:signal:) or init(initialValue:producer:).

The lifetime of transformed property - currently only map(transform:) - is unaffected.

@andersio

This comment has been minimized.

Show comment
Hide comment
@andersio

andersio May 25, 2016

Member

I have reverted the changes in the protocols, and would perhaps push them in a future PR. I am sorry for my attempt to pack everything in the one PR.

This PR now focuses on the property composition operators.

Member

andersio commented May 25, 2016

I have reverted the changes in the protocols, and would perhaps push them in a future PR. I am sorry for my attempt to pack everything in the one PR.

This PR now focuses on the property composition operators.

@andersio andersio changed the title from Property composition and enhancements. to Property composition operators. May 25, 2016

@andersio

This comment has been minimized.

Show comment
Hide comment
@andersio

andersio May 25, 2016

Member

@mdiep I wonder if the team might be interested in pushing these addictive operators into a Swift 2.2 release? If yes, I can take the breaking change out as a separated PR.

Member

andersio commented May 25, 2016

@mdiep I wonder if the team might be interested in pushing these addictive operators into a Swift 2.2 release? If yes, I can take the breaking change out as a separated PR.

Added a test case for `AnyProperty`.
It fails if the event is sent before the latest value is observable via `value`.
@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep May 25, 2016

Contributor

I have reverted the changes in the protocols, and would perhaps push them in a future PR. I am sorry for my attempt to pack everything in the one PR.

This PR now focuses on the property composition operators.

That's great—thank you! Smaller PRs are much easier to review. I'm happy to consider those other changes in a separate PR; I was trying to figure out how they related to the property composition operators.

I wonder if the team might be interested in pushing these addictive operators into a Swift 2.2 release? If yes, I can take the breaking change out as a separated PR.

Yes, I'd be interested in that. It seems like the breaking changes were unrelated to property composition.

Contributor

mdiep commented May 25, 2016

I have reverted the changes in the protocols, and would perhaps push them in a future PR. I am sorry for my attempt to pack everything in the one PR.

This PR now focuses on the property composition operators.

That's great—thank you! Smaller PRs are much easier to review. I'm happy to consider those other changes in a separate PR; I was trying to figure out how they related to the property composition operators.

I wonder if the team might be interested in pushing these addictive operators into a Swift 2.2 release? If yes, I can take the breaking change out as a separated PR.

Yes, I'd be interested in that. It seems like the breaking changes were unrelated to property composition.

@andersio andersio closed this May 25, 2016

@andersio andersio reopened this May 25, 2016

@andersio

This comment has been minimized.

Show comment
Hide comment
@andersio

andersio May 26, 2016

Member

Please note that the implementation assumes observers of a signal are called in the insertion order, which is the current behaviour of Bag.

A test case for this assumption (e3215a5) has been added, in case there is any change to Bag in the future.

Member

andersio commented May 26, 2016

Please note that the implementation assumes observers of a signal are called in the insertion order, which is the current behaviour of Bag.

A test case for this assumption (e3215a5) has been added, in case there is any change to Bag in the future.

return nil
}
/// A read-only, type-erased view of a property.

This comment has been minimized.

@mdiep

mdiep May 30, 2016

Contributor

I don't think I'd describe this as type-erased.

@mdiep

mdiep May 30, 2016

Contributor

I don't think I'd describe this as type-erased.

This comment has been minimized.

@andersio

andersio Jun 1, 2016

Member

It erased the type of what actually PropertyType is. Though for transformed property the description is off.

Examples: https://github.com/apple/swift/blob/master/stdlib/public/core/ExistentialCollection.swift.gyb

@andersio

andersio Jun 1, 2016

Member

It erased the type of what actually PropertyType is. Though for transformed property the description is off.

Examples: https://github.com/apple/swift/blob/master/stdlib/public/core/ExistentialCollection.swift.gyb

Show outdated Hide outdated ReactiveCocoa/Swift/Property.swift Outdated
Show outdated Hide outdated ReactiveCocoa/Swift/Property.swift Outdated
Show outdated Hide outdated ReactiveCocoa/Swift/Property.swift Outdated
Show outdated Hide outdated ReactiveCocoa/Swift/Property.swift Outdated
Show outdated Hide outdated ReactiveCocoa/Swift/Property.swift Outdated
Show outdated Hide outdated ReactiveCocoa/Swift/Property.swift Outdated
Show outdated Hide outdated ReactiveCocoa/Swift/Property.swift Outdated
@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep May 30, 2016

Contributor

Thank you for your continued work on this!

Contributor

mdiep commented May 30, 2016

Thank you for your continued work on this!

@andersio andersio closed this Jun 1, 2016

@andersio andersio reopened this Jun 1, 2016

@andersio andersio closed this Jun 3, 2016

@andersio andersio reopened this Jun 3, 2016

@andersio

This comment has been minimized.

Show comment
Hide comment
@andersio

andersio Jun 3, 2016

Member

@mdiep Hmm... I am wrong about Concat. It is behaving normally in my WIP test cases, and should be fine to be introduced to PropertyType.

Sorry for any confusion caused. :-[

Member

andersio commented Jun 3, 2016

@mdiep Hmm... I am wrong about Concat. It is behaving normally in my WIP test cases, and should be fine to be introduced to PropertyType.

Sorry for any confusion caused. :-[

@andersio andersio closed this Jun 3, 2016

@andersio andersio reopened this Jun 3, 2016

@mdiep

This comment has been minimized.

Show comment
Hide comment
@mdiep

mdiep Jun 5, 2016

Contributor

Looks good to me. Thanks for all your work on this!

Contributor

mdiep commented Jun 5, 2016

Looks good to me. Thanks for all your work on this!

@mdiep mdiep merged commit 8e2f364 into ReactiveCocoa:RAC5 Jun 5, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@andersio andersio deleted the andersio:RAC5-property branch Jun 6, 2016

@mdiep mdiep referenced this pull request Jun 16, 2016

Merged

RAC 5.0 #2857

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment