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

Added bind(to:) methods for PublishRelay and BehaviorRelay. Added relevant tests for binding. #1470

Merged
merged 1 commit into from Nov 26, 2017

Conversation

Projects
None yet
5 participants
@biboran
Contributor

biboran commented Oct 23, 2017

Binding implementation related to #1466.

Off-topic: why PublishRelay and BehaviorRelay don't conform to ObserverType?

@RxPullRequestBot

This comment has been minimized.

Show comment
Hide comment
@RxPullRequestBot

RxPullRequestBot Oct 23, 2017

1 Warning
⚠️ No CHANGELOG changes made

Generated by 🚫 Danger

RxPullRequestBot commented Oct 23, 2017

1 Warning
⚠️ No CHANGELOG changes made

Generated by 🚫 Danger

@inamiy

This comment has been minimized.

Show comment
Hide comment
@inamiy

inamiy Oct 25, 2017

Contributor

Off-topic: why PublishRelay and BehaviorRelay don't conform to ObserverType?

Observer can also receive Event.error which should be prohibited in any Variable-like types.

BTW if this PR is going to be merged, I'm wondering what will be the difference between BehaviorRelay and Variable besides SpinLock.
(Solved: BehaviorRelay conforms to ObservableType, so there's difference.)

And if sending error to relay class should be prohibited, bind(to:) should probably be prohibited too.

Contributor

inamiy commented Oct 25, 2017

Off-topic: why PublishRelay and BehaviorRelay don't conform to ObserverType?

Observer can also receive Event.error which should be prohibited in any Variable-like types.

BTW if this PR is going to be merged, I'm wondering what will be the difference between BehaviorRelay and Variable besides SpinLock.
(Solved: BehaviorRelay conforms to ObservableType, so there's difference.)

And if sending error to relay class should be prohibited, bind(to:) should probably be prohibited too.

@biboran

This comment has been minimized.

Show comment
Hide comment
@biboran

biboran Oct 25, 2017

Contributor

@inamiy It was not possible to use subscribe on a Variable and neither would be possible to use subscribe on BehaviorRelay and PublishRelay. subscribe is a very technical term in rx world (at least as I perceive it).

bind(to:) is just a convenience ioc for accepting event on ObserverType-like entities. I think it's quite ok for relays to assume filtering over incoming events in this context.

Contributor

biboran commented Oct 25, 2017

@inamiy It was not possible to use subscribe on a Variable and neither would be possible to use subscribe on BehaviorRelay and PublishRelay. subscribe is a very technical term in rx world (at least as I perceive it).

bind(to:) is just a convenience ioc for accepting event on ObserverType-like entities. I think it's quite ok for relays to assume filtering over incoming events in this context.

@inamiy

This comment has been minimized.

Show comment
Hide comment
@inamiy

inamiy Oct 25, 2017

Contributor

@biboran
I thought current BehaviorRelay can only be bound by Driver for safe binding (since type guarantees it won't emit error), and the impl was on purpose.

Yes, as you said "quite ok to assume", we can go unsafe way to let any observables bind to relay classes, but that's almost the same as going back to previous bind(to: variable), so I was just curious about that.

Contributor

inamiy commented Oct 25, 2017

@biboran
I thought current BehaviorRelay can only be bound by Driver for safe binding (since type guarantees it won't emit error), and the impl was on purpose.

Yes, as you said "quite ok to assume", we can go unsafe way to let any observables bind to relay classes, but that's almost the same as going back to previous bind(to: variable), so I was just curious about that.

@kzaher

This comment has been minimized.

Show comment
Hide comment
@kzaher

kzaher Nov 4, 2017

Member

I think that the most ideal solution would be if Swift allowed generic types with default generic parameters:

Observable<Element, Error = Never, Completed = Never>

... and parametrized error throwing.

func a<E>() throw E 

Then we could specify:

extension Observable where Error == Never, Completed == Never {
     func bind(to: PublishRelay<Element>)
}

Until then the only classes that have some guarantees are SharedSequence specializations called Driver and Signal.
Even binding them is wrong since one doesn't have a guarantee of not terminating, but it's usually good enough.

What should we do in ideal case is clear.

What should we do in this case isn't so much IMHO.

I've moved extensions that enable binding of Observable to Variable to Deprecated.

Binding of any observable that is finite or errors out is wrong IMHO, but right now we don't have compile time guarantees regarding those properties on Observable.

Member

kzaher commented Nov 4, 2017

I think that the most ideal solution would be if Swift allowed generic types with default generic parameters:

Observable<Element, Error = Never, Completed = Never>

... and parametrized error throwing.

func a<E>() throw E 

Then we could specify:

extension Observable where Error == Never, Completed == Never {
     func bind(to: PublishRelay<Element>)
}

Until then the only classes that have some guarantees are SharedSequence specializations called Driver and Signal.
Even binding them is wrong since one doesn't have a guarantee of not terminating, but it's usually good enough.

What should we do in ideal case is clear.

What should we do in this case isn't so much IMHO.

I've moved extensions that enable binding of Observable to Variable to Deprecated.

Binding of any observable that is finite or errors out is wrong IMHO, but right now we don't have compile time guarantees regarding those properties on Observable.

@biboran

This comment has been minimized.

Show comment
Hide comment
@biboran

biboran Nov 6, 2017

Contributor

That makes sense. I just read a bit about the intention of relays and recognized that binding Observables that might error out or complete is a terrible idea for a Relay.

What might make sense is binding a Relay to a Relay but I am not sure if it's needed or even a good architectural design pattern.

Contributor

biboran commented Nov 6, 2017

That makes sense. I just read a bit about the intention of relays and recognized that binding Observables that might error out or complete is a terrible idea for a Relay.

What might make sense is binding a Relay to a Relay but I am not sure if it's needed or even a good architectural design pattern.

@beeth0ven beeth0ven referenced this pull request Nov 7, 2017

Open

Cleanup public API #120

@inamiy

This comment has been minimized.

Show comment
Hide comment
@inamiy

inamiy Nov 7, 2017

Contributor

@kzaher
Thinking of completable observable binding as you mentioned, now I start to feel it's OK to merge this PR since bind(to:) is essentially unsafe anyway.
(Or maybe renaming to unsafeBind(to:) can bring developer's more attention)

For better typing, Completable is normally either Void or Never (which is not fun to make as a new type-param), so I would rather make Event as generic as follows (sorry for off-topic!):

https://gist.github.com/inamiy/de1a495ec198b7f9b504b9598999e6ed

Contributor

inamiy commented Nov 7, 2017

@kzaher
Thinking of completable observable binding as you mentioned, now I start to feel it's OK to merge this PR since bind(to:) is essentially unsafe anyway.
(Or maybe renaming to unsafeBind(to:) can bring developer's more attention)

For better typing, Completable is normally either Void or Never (which is not fun to make as a new type-param), so I would rather make Event as generic as follows (sorry for off-topic!):

https://gist.github.com/inamiy/de1a495ec198b7f9b504b9598999e6ed

@kzaher

This comment has been minimized.

Show comment
Hide comment
@kzaher

kzaher Nov 7, 2017

Member

Hi @inamiy ,

yeah, there are multiple ways how one could model observables and events, and each has its benefits and downsides.

We've so far used compositional approach because it was most practical and most backwards compatible. We could maybe figure out some even more typesafe approach, but this worked ok so far.

So far we don't have Infinite trait, but I guess one could have it.

I would like to find some nice way that provides more flexibility regarding enabling/disabling certain types of events, but unfortunately haven't figured out anything that pleases me.

I think that solution that you've proposed also unfortunately has some issues, but yeah, maybe there is some nice solution out there.

Member

kzaher commented Nov 7, 2017

Hi @inamiy ,

yeah, there are multiple ways how one could model observables and events, and each has its benefits and downsides.

We've so far used compositional approach because it was most practical and most backwards compatible. We could maybe figure out some even more typesafe approach, but this worked ok so far.

So far we don't have Infinite trait, but I guess one could have it.

I would like to find some nice way that provides more flexibility regarding enabling/disabling certain types of events, but unfortunately haven't figured out anything that pleases me.

I think that solution that you've proposed also unfortunately has some issues, but yeah, maybe there is some nice solution out there.

@inamiy

This comment has been minimized.

Show comment
Hide comment
@inamiy

inamiy Nov 8, 2017

Contributor

@kzaher
For non-breaking approach, adding Infinite trait (or whatever name is) seems the best solution so far, but I guess it should be discussed in other PR for now.

Since Driver.drive(behaviorRelay) is still unsafe due to completable event, I personally prefer to:

  1. Merge this PR without renaming (every binding is potentially unsafe anyway)
  2. Then, in future, introduce Infinite and either delete or rename other binding methods with unsafe- prefix (or just add warning documentation without breaking them).

I think as this because I see many developers adding the similar custom extensions like this PR to bring back to Variable world, and even Driver can't help with that perfectly.

What do you think?

Contributor

inamiy commented Nov 8, 2017

@kzaher
For non-breaking approach, adding Infinite trait (or whatever name is) seems the best solution so far, but I guess it should be discussed in other PR for now.

Since Driver.drive(behaviorRelay) is still unsafe due to completable event, I personally prefer to:

  1. Merge this PR without renaming (every binding is potentially unsafe anyway)
  2. Then, in future, introduce Infinite and either delete or rename other binding methods with unsafe- prefix (or just add warning documentation without breaking them).

I think as this because I see many developers adding the similar custom extensions like this PR to bring back to Variable world, and even Driver can't help with that perfectly.

What do you think?

@orakaro

This comment has been minimized.

Show comment
Hide comment
@orakaro

orakaro Nov 13, 2017

Bumped into this and 👍 for the idea of Infinitive trait. Till now I have to create infinitive sequence like

public func neverComplete() -> Observable<E> {
    return concat(Observable.never())
}

but have no guarantee to be called before binding to Subjects.

Another thing is, why there is implementation of drive to BehaviourRelay in Driver+Subscription, but no "drive to PublishRelay" method ?

orakaro commented Nov 13, 2017

Bumped into this and 👍 for the idea of Infinitive trait. Till now I have to create infinitive sequence like

public func neverComplete() -> Observable<E> {
    return concat(Observable.never())
}

but have no guarantee to be called before binding to Subjects.

Another thing is, why there is implementation of drive to BehaviourRelay in Driver+Subscription, but no "drive to PublishRelay" method ?

@kzaher

This comment has been minimized.

Show comment
Hide comment
@kzaher

kzaher Nov 15, 2017

Member

@inamiy

I think as this because I see many developers adding the similar custom extensions like this PR to bring back to Variable world, and even Driver can't help with that perfectly.

Yes, I agree. It's better to at least have those extensions in one place.

Then, in future, introduce Infinite and either delete or rename other binding methods with unsafe- prefix (or just add warning documentation without breaking them).

I think it's best to not add Infinite in this repo. I believe one would create only marginal improvement regarding safety. Consider this case:

Observable.just("This is not wanter")
.concat(Observable.never())

I doesn't complete, but does not sending Complete doesn't prove there can ever be another element sent. Or consider this case.

Observable.error(TestError())
.catchError { e in
    Observable.never()
}

@orakaro

Another thing is, why there is implementation of drive to BehaviourRelay in Driver+Subscription, but no "drive to PublishRelay" method ?

Because of different sharing strategies. share vs share(replay: 1).

Member

kzaher commented Nov 15, 2017

@inamiy

I think as this because I see many developers adding the similar custom extensions like this PR to bring back to Variable world, and even Driver can't help with that perfectly.

Yes, I agree. It's better to at least have those extensions in one place.

Then, in future, introduce Infinite and either delete or rename other binding methods with unsafe- prefix (or just add warning documentation without breaking them).

I think it's best to not add Infinite in this repo. I believe one would create only marginal improvement regarding safety. Consider this case:

Observable.just("This is not wanter")
.concat(Observable.never())

I doesn't complete, but does not sending Complete doesn't prove there can ever be another element sent. Or consider this case.

Observable.error(TestError())
.catchError { e in
    Observable.never()
}

@orakaro

Another thing is, why there is implementation of drive to BehaviourRelay in Driver+Subscription, but no "drive to PublishRelay" method ?

Because of different sharing strategies. share vs share(replay: 1).

@kzaher kzaher merged commit bbd2ddc into ReactiveX:develop Nov 26, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment