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

OuterSubscriber::notifyNext() to pass its innersubscriber? #1250

Closed
kwonoj opened this issue Jan 25, 2016 · 5 comments
Closed

OuterSubscriber::notifyNext() to pass its innersubscriber? #1250

kwonoj opened this issue Jan 25, 2016 · 5 comments
Assignees

Comments

@kwonoj
Copy link
Member

kwonoj commented Jan 25, 2016

I'm not sure if this is legit, please consider it as speaking out my thought - feel freely close if it's not appropriate.

OuterSubscriber 's interface for error and complete passes its innersubscription when it occurs, as

notifyError(error: any, innerSub: InnerSubscriber<T, R>)
notifyComplete(innerSub: InnerSubscriber<T, R>)
notifyNext(outerValue: T, innerValue: R, outerIndex: number, innerIndex: number)

while notifyNext doesn't. Came to think align interfaces notifyNext also, for some cases of support early-unsubscription as soon as inner emits like delayWhen, i.e

source              : --a--b--|
selector            :   -x--|
                           --x----|
selectorSubscription:   ^!
                           ^ !

subscription to selector observable unsubscribes as soon as selector emits, or either completes without emitting. If notifyNext passes its subscription, outer subscriber doesn't have to holds reference or need to lookup to manage subscription, allows snippet like

notifyNext(..., innseSub) {
//do things
innerSub.unsubscribe();
this.remove(innerSub);
}
@trxcllnt
Copy link
Member

@kwonoj I remember wanting this a while back as well, so I'm in favor of this change.

@kwonoj
Copy link
Member Author

kwonoj commented Jan 25, 2016

I can gladly take effort to create PR if this reaches agreed to have.

@benlesh
Copy link
Member

benlesh commented Jan 26, 2016

I'm indifferent. Just be sure to test perf before and after.

@kwonoj
Copy link
Member Author

kwonoj commented Jan 26, 2016

Cool, I'll come up PR with perf comparison to see if this change does not hurt existing operator behavior.

@kwonoj kwonoj self-assigned this Jan 26, 2016
kwonoj added a commit to kwonoj/rxjs that referenced this issue Jan 26, 2016
- notifyNext() passes inner subscriber as same as notifyError(), notifyComplete() does, supports subscription management

closes ReactiveX#1250
kwonoj added a commit to kwonoj/rxjs that referenced this issue Jan 27, 2016
- notifyNext() passes inner subscriber as same as notifyError(), notifyComplete() does, supports subscription management

closes ReactiveX#1250
@kwonoj kwonoj closed this as completed in 1df8928 Jan 27, 2016
@lock
Copy link

lock bot commented Jun 7, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants