ReactiveX / RxSwift

Reactive Programming in Swift
MIT License
24.32k stars 4.17k forks source link

`bind` function may cause ambiguity #2599

Closed harryzjm closed 4 months ago

harryzjm commented 4 months ago

RxCocoa 6.7.1 RxSwift 6.7.1 Xcode 15.3

extension ObservableType {
    public func bind<Result>(to binder: (Self) -> Result) -> Result {
        binder(self)
    }

    public func bind(onNext: @escaping (Element) -> Void) -> Disposable {
        self.subscribe(onNext: onNext,
                       onError: { error in
                        rxFatalErrorInDebug("Binding error: \(error)")
                       })
    }
}

This two function may cause ambiguity in some cases, such as:

let ry = BehaviorRelay<Bool>(value: false)
override func viewDidLoad() {
    super.viewDidLoad()

    _ = ry.bind { [weak self] bl in
        self?.test()
    }
}

func test() {}

In this case, the test method will be executed synchronously, don`t as I expect bind to test function if ry changed So I suggest change the name of the func bind<Result>(to binder: (Self) -> Result) -> Result method

danielt1263 commented 4 months ago

Interesting... As a side note though, ignoring the return value in this case is a guaranteed memory leak, and assigning it to a dispose bag will resolve the issue regarding which method will be used.

So, I agree with you in theory, but in practice, this isn't an issue.

harryzjm commented 4 months ago

yeah, In our project, there have .take(1) in the operators, so we often ignore the result, don`t assigning it to dispose bag.

danielt1263 commented 4 months ago

I personally have always used .bind(onNext: { }) so I've never noticed the issue...

harryzjm commented 4 months ago

You are right, But I just like less code, May be I should write more clearly.

freak4pc commented 4 months ago

Hey there, I don't really see how this is an issue with RxSwift :)

It's not ambiguity if you choose to use a possibly wrong overload. We also can't (and won't) rename methods that are part of the library for a very long time.

My main suggestion would be that if you have a very specific use case where you use take(1) a lot, etc, perhaps write your own extension on Observable for your own project. I'm not entirely sure this fits in the scope of fixing anything inside the project.

Thanks for the suggestion though!