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

Operators for binding Disposable to a Lifetime. #421

Closed
andersio opened this issue May 31, 2017 · 9 comments
Closed

Operators for binding Disposable to a Lifetime. #421

andersio opened this issue May 31, 2017 · 9 comments
Labels

Comments

@andersio
Copy link
Member

andersio commented May 31, 2017

In #334, we replaced CompositeDisposable with Lifetime for collecting cleanup callbacks in producers, and += is also deprecated. So for now adding a Disposable to Lifetime has to be written as:

// Two-liner
let disposable = self.start(observer)
lifetime.observeEnded(disposable.dispose)

// One-liner
lifetime.observeEnded(self.start(observer).dispose)

// Previously
disposable += self.start(observer)

If #404 is to be merged, the lack of an operator causes it even nastier for Signals.

// Two-liner
let disposable = self.observe(observer) // Optional
_ = (disposable?.dispose).map(lifetime.observeEnded)

if let disposable = self.observe(observer) {
    lifetime.observeEnded(disposable.dispose)
}

// Previously
disposable += self.observe(observer)

// Or
return self.observe(observer)

It makes me wonder if we can reintroduce an operator shorthand, but instead of += we would reuse <~.

disposable <~ lifetime

self.observe { event in
    // (event manipulation)
} <~ lifetime

This makes sense if we treat Disposable as a binding target of (). But on the other hand the LHS/RHS of the operator has to be swapped as compared to +=.

@mdiep
Copy link
Contributor

mdiep commented Jun 1, 2017

Moving lifetime to the rhs does't seem to help with readability either.

I'll keep this in mind when I review #404 and then we can discuss more. We should definitely do something to improve things if we're going to be merge that PR.

@ikesyo
Copy link
Member

ikesyo commented Jul 4, 2017

Coming from https://github.com/Carthage/ReactiveTask/pull/97/files#r125483012 and 👍 for some sort of operator or something like Disposable.bind(to: Lifetime) (over Lifetime.attach).

@mdiep
Copy link
Contributor

mdiep commented Jul 5, 2017

I'm on board with an operator. This suggestion from @andersio seems like it might make sense:

disposable <~ lifetime

@ikesyo
Copy link
Member

ikesyo commented Jul 6, 2017

This will be difficult to read / understand and will not be natural:

stderrAggregated
    .then(stdoutAggregated)
    .map(TaskEvent.success)
    .start(observer) <~ lifetime

The following (placing a lifetime on LHS) is preferable for me (as a natural replacement of += operator):

lifetime <~ stderrAggregated
    .then(stdoutAggregated)
    .map(TaskEvent.success)
    .start(observer)

@andersio
Copy link
Member Author

andersio commented Jul 6, 2017

The problem is that the inconsistent direction. <~ is used to have the observer as LHS and the source as RHS. lifetime <~ disposable is the inverse of this.

@mdiep
Copy link
Contributor

mdiep commented Jul 6, 2017

Okay:

  1. There should be an operator
  2. It should be (Lifetime, Disposable) -> Void

Maybe we should go back to +=? Does anyone have a better suggestion?

@sharplet
Copy link
Contributor

sharplet commented Jul 6, 2017

I'm pretty happy with +=, and was kinda sad to see it go. I do agree we should have a method on Disposable (a la. RxSwift's disposed(by: disposeBag)).

@jjoelson
Copy link
Contributor

jjoelson commented Jul 7, 2017

Is there any reason it couldn't be ~> to imply the opposite observer/source relationship as <~?

I also think += would be fine, as what else would you be adding to a Lifetime but something to be disposed when it ends?

@andersio
Copy link
Member Author

Implemented in #482.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants