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

Overload <~ to automatically bridge values to Objective-C #2551

Merged
merged 2 commits into from May 5, 2016
Merged

Overload <~ to automatically bridge values to Objective-C #2551

merged 2 commits into from May 5, 2016

Conversation

sharplet
Copy link
Contributor

There seems to a good amount of evidence that doing this kind of thing is no fun:

DynamicProperty(object: myLabel, keyPath: "text") <~ gimmeStrings.map { $0 } // 😞

I worked out a way to overload <~ for types that are _ObjectiveCBridgeable and automatically bridge them.

DynamicProperty(object: myLabel, keyPath: "text") <~ gimmeStrings // \o/

I'm not sure how I feel about the use of _ObjectiveCBridgeable and _bridgeToObjectiveC(). But I think this is pretty nice to have, and it should be equivalent to the implicitly bridged version.


P.S.

I didn't include it in the PR, but I also add this function to my project which brings it back to RAC 2 levels of awesomeness:

func RAC(object: NSObject?, _ keyPath: String) -> DynamicProperty {
  return DynamicProperty(object: object, keyPath: keyPath)
}

RAC(myLabel, "text") <~ gimmeStrings

@neilpa
Copy link
Member

neilpa commented Nov 12, 2015

While this is cool, I'm 👎 on adding it. Each additional operator overload almost always creates a significant negative impact to compile times. I've seen exponential or worse degradations per-overload on other projects.

I also don't want to encourage further use of DynamicProperty because it effectively circumvents Swift's type safety. To that end, I've been maintaining Rex which at this point is mostly about providing type-safe cocoa bindings. At this point these should probably be moved into RAC but I haven't found the time to port them, flush out the missing properties, add documentation, etc.

@sharplet
Copy link
Contributor Author

Each additional operator overload almost always creates a significant negative impact to compile times. I've seen exponential or worse degradations per-overload on other projects.

Are you referring to ReactiveCocoa compile times, or compile times for callers of the operator?

@neilpa
Copy link
Member

neilpa commented Nov 12, 2015

Existing usage of the operator

@neilpa
Copy link
Member

neilpa commented Nov 12, 2015

Although I'll admit that my data on compile-time increases when calling overloaded operators is anecdotal from twitter and other github issues.

@NachoSoto
Copy link
Member

Thanks for the PR!

I also don't want to encourage further use of DynamicProperty because it effectively circumvents Swift's type safety.

For sure, but we need to improve the current situation because right now it's really hard to use with UIKit/AppKit.

I think this is interesting too, but it only solves a tiny portion of the problems. What I brought up in #2535 was a more general perspective of the issue, which how hard to compose PropertyType is. If we can solve that problem in a general way then we'll have a framework to improve things like this :)

@sharplet
Copy link
Contributor Author

So, for posterity at least, I've just pushed a commit that adds one more overload for binding a source PropertyType with bridgeable values to DynamicProperty.

Compile-time performance is really important, so I think it would be really interesting to measure the difference these overloads would make to a project that uses a lot of bindings. Validating our assumptions there could at least be data to back a radar about compile-time performance for overloaded operators.

I don't feel like my current project is of a sufficient size to really validate the performance question (at least at its current early stage). Is there perhaps anyone we could invite to try out this branch and report back?

@sharplet
Copy link
Contributor Author

Also, regarding the future design questions about composition of properties, is the hope to solve that for RAC 4 (in case breaking changes are required)? In my use, I'm finding that it adds a lot of value to my codebase in terms of readability, so I think it would be great to have some kind of enhancement available in 4.0.

@RuiAAPeres
Copy link
Member

@neilpa @NachoSoto should this be merged or closed?


/// Binds a signal producer to a `DynamicProperty`, automatically bridging values to Objective-C.
public func <~ <Value: _ObjectiveCBridgeable>(property: DynamicProperty, producer: SignalProducer<Value, NoError>) -> Disposable {
return property <~ producer.map { $0._bridgeToObjectiveC() }
Copy link
Contributor

Choose a reason for hiding this comment

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

The indentation here and above is off. RAC uses tabs. ☺️

@mdiep
Copy link
Contributor

mdiep commented Apr 28, 2016

I agree both that (a) the current situation is not great and (b) the type-unsafely of DynamicProperty is not great. Furthermore, I think @NachoSoto's idea to make DynamicProperty generic over Value (as proposed in #2535) is a great one. AIUI, that would eliminate the need for the operators proposed here.

In light of that, I'd suggest:

  1. That we merge these operators to help with the situation in RAC 4.
  2. That we make DynamicProperty generic over Value for RAC 5 and remove these operators.

What does everyone think of that? @sharplet, would you be interested in opening a PR for (2)?

@sharplet
Copy link
Contributor Author

@mdiep I've fixed up the indentation and rebased on master. I took a stab at (2), and I'll be pushing a PR for it shortly!

@sharplet
Copy link
Contributor Author

@mdiep I've opened #2838, with the breaking changes to DynamicProperty.

@ikesyo
Copy link
Member

ikesyo commented May 4, 2016

#2852 is merged for RAC 5 with breaking changes, but should this still be merged for current RAC 4?

@mdiep
Copy link
Contributor

mdiep commented May 5, 2016

👍 to merging this. Let's get the CI build to pass though.

@sharplet
Copy link
Contributor Author

sharplet commented May 5, 2016

Looks like the build passed on the last commit?

@mdiep
Copy link
Contributor

mdiep commented May 5, 2016

If failed the first time, but I reran it. 😄

@mdiep mdiep merged commit 8eb6e07 into ReactiveCocoa:master May 5, 2016
@sharplet
Copy link
Contributor Author

sharplet commented May 5, 2016

Excellent, thanks everyone!

@sharplet sharplet deleted the dynamic-sugar branch May 5, 2016 03:24
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

6 participants