RxSwiftCommunity / RxSwiftExt

A collection of Rx operators & tools not found in the core RxSwift distribution
MIT License
1.32k stars 213 forks source link

Add SharedSequence weak subscriptions #236

Closed mpsnp closed 4 years ago

mpsnp commented 5 years ago

This pull request adds both Signal & Driver weak subscriptions.

rxswiftcommunity[bot] commented 5 years ago
Warnings
:warning: It looks like code was changed without adding anything to the Changelog. If this is a trivial PR that doesn't need a changelog, add #trivial to the PR title or body.

Generated by :no_entry_sign: dangerJS

freak4pc commented 5 years ago

Hey @mpsnp - Thanks for your contribution.

Is this really different then withUnretained + subscribe ? I'd prefer not add more operators unless needed.

M0rtyMerr commented 5 years ago

Same question here

Sent with GitHawk

mpsnp commented 5 years ago

Ok, here is my explanation:

  1. withUnretained + subscribe only works for Observable, so shared sequences should be converted to Observable first, loosing their original types, which is not good i guess
  2. There are already extensions for Observable which do exactly the same as in this PR.
  3. Using curried style is preferred in my opinion, because it does not require to wrap a call of "unretained" object function to closure or uncurry its method.
    Just compare usage:
    state // Usually Driver
        .asObservable()
        .withUnretained(self)
        .subscribe(onNext: { controller, state in 
            controller.render(state: state)
        })
        .disposed(by: disposeBag)

    with

    state
        .drive(weak: self, onNext: Controller.render)
        .disposed(by: disposeBag)
mpsnp commented 4 years ago

@freak4pc @MortyMerr any feedback? Actually this is not "new" operator, it already exists here but only for ObservableType. I just wanted to add it to SharedSequence. IMHO withUnretained also should be added to Traits, what do you think?

M0rtyMerr commented 4 years ago

Please also note this PR with corresponding issue https://github.com/RxSwiftCommunity/RxSwiftExt/pull/219 For me subscribe(weak: self, onNext: ...) and withUnretanied(self).subscribe(onNext:) purpose looks absolutely the same.

I think that these operators should be merged into one (for all traits). One operator for weakly referencing an object in subscription block is more than enough.

I prefer withUnretained, cause it is more generic. It can be used not only with subscription, but with map, do etc.

subscribe(weak: is too specific case, I think it's senseless to have own wrapper for all RxSwift operators (do(weak:, map(weak:, filter(weak:)

What do you think?

freak4pc commented 4 years ago

I'm siding with Anton here — I'm seeing very little value adding a bunch of do something with weak object overloads when we have withUnretained that solves all of these cases. Yes, there is an existing subscribe(onNext:) but it precedes withUnretained by a years, probably :) There's no reason to deprecate it since it's already in the library, but there's no reason to add more of it with a better solution available.

Currying-style doesn't seem like a good enough reason (to me) to add a bunch of these overloads.

M0rtyMerr commented 4 years ago

What we actually can do is to add withUnretained for all traits