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

[RxCocoa] Move from `rx_` prefix to a `rx.` proxy (for Swift 3 update ?) #826

Closed
jegnux opened this Issue Aug 8, 2016 · 6 comments

Comments

Projects
None yet
4 participants
@jegnux
Contributor

jegnux commented Aug 8, 2016

Description

The rx_ prefix of all reactive extensions to Cocoa are not really pretty and don't fit with the Swift coding guidelines. They are multiple choices to rid off of this syntax :

  • rx_tap : The original syntax
  • rxTap : Standard camel case with rx prefix but it feels pretty weird to have several method starting with rx. Remember, rx means Reactive Extension so it's read as reactive extension tap.
  • reactiveTap : Still camel case but with a more readable and meaningful name. But it's clearly less convenient than the rx prefix
  • rx.tap : 😲 Whouah, this one is so beautiful. A nice pretty namespace to all reactive extensions of an object. It's as readable as it's convenient to write in autocompleted environment.
Why rx. prefix is a good design imho ?

The main idea is to use a proxy which own the original object. Then we extend the proxy to implement a new reactive extension.

You can see this kind of pattern in the Switf Standard Library with the LazySequence type. For instance if you have an array myArray, you can perform a map directly or through it's lazy proxy :

myArray.map { ... }
myArray.lazy.map { ... }

We could have the same syntax in RxCocoa :

if myButton.enabled { ... }
myButton.rx.enabled.subscribeNext { ... }

I kinda like this syntax, and I wrote a post on medium about how to use this syntax to add a safe subscripting for CollectionType. Indeed, the common solution I used to see is to add an extension to be able to write something like

if let element = myArray[safe: 2] { ...

While I prefer the proxy pattern to be able to write this :

if let element = myArray.safe[2] { ... 

Implementation details

Shared code

I suggest we could transform the Reactive protocol to a generic struct :

public struct Reactive<Base: AnyObject> {
    public let base: Base
    public init(_ base: Base) {
        self.base = base
    }
}

Then we extend NSObjectProtocol to create this rx proxy :

public extension NSObjectProtocol {
    public var rx: Reactive<Self> {
        return Reactive(self)
    }
}

Because we use Self we need to extend a protocol instead of a concrete type. I suggest to use NSObjectProtocol for convenience, but we could also use a more meaningful protocol like ReactiveCompatible or something like that.

Actual extensions

Now to add reactive extensions to a type, we need to extend the Reactive with a generic constraint instead of extending the target type itself.

Thus, for instance, this code :

extension UIButton {
    public var rx_tap: ControlEvent<Void> {
        return rx_controlEvent(.touchUpInside)
    }
}

will be replaced by this code :

extension Reactive where Base: UIButton {
    public var tap: ControlEvent<Void> {
        return controlEvent(.touchUpInside)
    }
}

Memory Management Considerations

The main drawback I see to this new syntax, is that it could lead to a less intuitive memory management inside closures.

Indeed, now, reactive extensions are on the Reactive type (the proxy). So wherever you use self in a method implementation, it refers to the proxy instead of the base.
But as the proxy is a value type, you can't weakify it, and everytime you use self inside a closure, it actually strongify (retain) all of its reference type member (ie: the base).

So you need to be extra careful on how you implement the extension.
Let's take the example of UISearchBar.

Sample 1 : current implementation

1.  extension UISearchBar {
2.      public var rx_delegate: DelegateProxy {
3.          return RxSearchBarDelegateProxy.proxyForObject(self)
4.      }
5.  
6.      public var rx_text: ControlProperty<String> {
7.          let source: Observable<String> = Observable.deferred { [weak self] () -> Observable<String> in
8.              let text = self?.text ?? ""
9.              
10.             return (self?.rx_delegate.observe(#selector(UISearchBarDelegate.searchBar(_:textDidChange:))) ?? Observable.empty())
11.                     .map { a in
12.                         return a[1] as? String ?? ""
13.                     }
14.                     .startWith(text)
15.         }
16. 
17.         let bindingObserver = UIBindingObserver(UIElement: self) { (searchBar, text: String) in
18.             searchBar.text = text
19.         }
20.         
21.         return ControlProperty(values: source, valueSink: bindingObserver)
22.     }
23. }

Sample 2 : new implementation with wrong memory management

1.  extension Reactive where Base: UISearchBar {
2.      public var delegate: DelegateProxy {
3.          return RxSearchBarDelegateProxy.proxyForObject(self.base)
4.      }
5.  
6.      public var text: ControlProperty<String> {
7.          let source: Observable<String> = Observable.deferred { [weak searchBar = self.base] () -> Observable<String> in
8.              let text = searchBar?.text ?? ""
9.              
10.             return self.delegate.observe(#selector(UISearchBarDelegate.searchBar(_:textDidChange:)))
11.                     .map { a in
12.                         return a[1] as? String ?? ""
13.                     }
14.                     .startWith(text)
15.         }
16. 
17.         let bindingObserver = UIBindingObserver(UIElement: self.base) { (searchBar, text: String) in
18.             searchBar.text = text
19.         }
20.         
21.         return ControlProperty(values: source, valueSink: bindingObserver)
22.     }
23. }

Sample 3 : new implementation with good memory management

1.  extension Reactive where Base: UISearchBar {
2.      public var delegate: DelegateProxy {
3.          return RxSearchBarDelegateProxy.proxyForObject(self.base)
4.      }
5.  
6.      public var text: ControlProperty<String> {
7.          let source: Observable<String> = Observable.deferred { [weak searchBar = self.base] () -> Observable<String> in
8.              let text = searchBar?.text ?? ""
9.              
10.             return (searchBar?.rx.delegate.observe(#selector(UISearchBarDelegate.searchBar(_:textDidChange:))) ?? Observable.empty())
11.                     .map { a in
12.                         return a[1] as? String ?? ""
13.                     }
14.                     .startWith(text)
15.         }
16. 
17.         let bindingObserver = UIBindingObserver(UIElement: self.base) { (searchBar, text: String) in
18.             searchBar.text = text
19.         }
20.         
21.         return ControlProperty(values: source, valueSink: bindingObserver)
22.     }
23. }

The critical part is on line 10 of each sample.
In the sample 2 we can be tempted to access to the delegate observer through self because delegate is defined on the same proxy extension. But by doing so, self is captured by the closure and all of its reference member are retained. So it cancel the [weak searchBar = self.base] capture list :-/

Th correct way to avoid this is to always replace self in old implementation by the base, and everytime there is the rx_ prefix it should be replaced by the rx. proxy (see Sample 3). This way, it should be fine :)

Conclusion

I really hope you'll like this proposal. If approved, the update to the Swift 3 syntax could be a great time to make this move as we'll all have a lot of work to update our codebase to Swift 3 anyway. So a little bit more or a little bit less... it doesn't matter :p

I already started to work on this for a future pull request, but I'd really like to have the community feedback on this.
Thanks for reading :)

@kzaher

This comment has been minimized.

Show comment
Hide comment
@kzaher

kzaher Aug 8, 2016

Member

Hi @jegnux ,

awesome proposal 🎉

I would really like to see this implemented.

As far as for memory management, it seems to me that it would be easier to accidentally cause a strong reference to extended object, but even in that case, strong reference shouldn't cause a leak in case dispose bag is used.

Maybe it would be a good idea to name Reactive differently. Maybe RxProxy. I don't really like RxProxy either, but don't have a better suggestion.

My brain just replaces the word "reactive" in programming context with callback, so I'm little confused when reading it :) I'm aware of reactive manifesto, but even for people who have read it, I don't think the word itself brings any clarity in this context :) Also, there are extensions that aren't observable sequences, so that might also be a reason against. I'm also curious about other suggestions. I guess if people like Reactive that much, I would be fine with it.
¯_(ツ)_/¯

Maybe it would also be good idea to rename base to source? The word base implies to me some form of inheritance, and we are using source terminology when extending ObservableType. Also open to suggestions.

Member

kzaher commented Aug 8, 2016

Hi @jegnux ,

awesome proposal 🎉

I would really like to see this implemented.

As far as for memory management, it seems to me that it would be easier to accidentally cause a strong reference to extended object, but even in that case, strong reference shouldn't cause a leak in case dispose bag is used.

Maybe it would be a good idea to name Reactive differently. Maybe RxProxy. I don't really like RxProxy either, but don't have a better suggestion.

My brain just replaces the word "reactive" in programming context with callback, so I'm little confused when reading it :) I'm aware of reactive manifesto, but even for people who have read it, I don't think the word itself brings any clarity in this context :) Also, there are extensions that aren't observable sequences, so that might also be a reason against. I'm also curious about other suggestions. I guess if people like Reactive that much, I would be fine with it.
¯_(ツ)_/¯

Maybe it would also be good idea to rename base to source? The word base implies to me some form of inheritance, and we are using source terminology when extending ObservableType. Also open to suggestions.

@mohsenr

This comment has been minimized.

Show comment
Hide comment
@mohsenr

mohsenr Aug 9, 2016

Contributor

👍!

FWIW I prefer Reactive to RxProxy. The latter reads like it has an objective-c style class prefix.

Contributor

mohsenr commented Aug 9, 2016

👍!

FWIW I prefer Reactive to RxProxy. The latter reads like it has an objective-c style class prefix.

@kzaher

This comment has been minimized.

Show comment
Hide comment
@kzaher

kzaher Aug 9, 2016

Member

I've talked with @jegnux on slack, and he also doesn't like RxProxy.

Ok, fine :) Let's use Reactive for now. We can change this if we figure out some better name.
It does have a hidden pun extension Reactive.
¯_(ツ)_/¯

What about source vs base vs something else?

Member

kzaher commented Aug 9, 2016

I've talked with @jegnux on slack, and he also doesn't like RxProxy.

Ok, fine :) Let's use Reactive for now. We can change this if we figure out some better name.
It does have a hidden pun extension Reactive.
¯_(ツ)_/¯

What about source vs base vs something else?

@jegnux

This comment has been minimized.

Show comment
Hide comment
@jegnux

jegnux Aug 10, 2016

Contributor

I prefer Reactive over RxProxy because it reads better (but this is maybe because I always read rx as reactive extension)
Reactive<UIButton> is a Reactive version of UIButton.
RxProxy<UIButton> is a Reactive Extension Proxy of UIButton which is a little bit 🤔

For source over base, I'm not such uncompromising. Indeed, source seems more natural to me, but Apple use base for its LazyCollectionSequence.
Moreover, as source is already used as terminology when extending ObservableType, it were a little bit confusing when I started to perform changes with the source naming because the local source declaration was shadowing the instance var.
So i preferred to use something more distinct.

Contributor

jegnux commented Aug 10, 2016

I prefer Reactive over RxProxy because it reads better (but this is maybe because I always read rx as reactive extension)
Reactive<UIButton> is a Reactive version of UIButton.
RxProxy<UIButton> is a Reactive Extension Proxy of UIButton which is a little bit 🤔

For source over base, I'm not such uncompromising. Indeed, source seems more natural to me, but Apple use base for its LazyCollectionSequence.
Moreover, as source is already used as terminology when extending ObservableType, it were a little bit confusing when I started to perform changes with the source naming because the local source declaration was shadowing the instance var.
So i preferred to use something more distinct.

@damianesteban

This comment has been minimized.

Show comment
Hide comment
@damianesteban

damianesteban Aug 15, 2016

Contributor

👍 for Reactive. It definitely reads much better and looks cleaner.

Contributor

damianesteban commented Aug 15, 2016

👍 for Reactive. It definitely reads much better and looks cleaner.

@kzaher

This comment has been minimized.

Show comment
Hide comment
@kzaher

kzaher Aug 18, 2016

Member

Ok, I've merged PR. We'll keep all original proposed names. 👍

Member

kzaher commented Aug 18, 2016

Ok, I've merged PR. We'll keep all original proposed names. 👍

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