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

[WIP] fix: ensure all interop operators are chained correctly #5243

Closed

Conversation

cartant
Copy link
Collaborator

@cartant cartant commented Jan 18, 2020

WIP:

#5333 is likely a better solution to the problem.


Description:

This PR refactors the fix in #5239 so that it can be applied to all operators - or at least to those that need unsubscription to be chained.

The problem is outlined in the linked issue, but AFAICT the problem is not specific to the finalize operator. For example, the buffer operator's subscriber adds a subscription that depends upon unsubscription being chained correctly:

constructor(destination: Subscriber<T[]>, closingNotifier: Observable<any>) {
super(destination);
this.add(subscribeToResult(this, closingNotifier));
}

And windowTime adds scheduled actions:

constructor(protected destination: Subscriber<Observable<T>>,
private windowTimeSpan: number,
private windowCreationInterval: number | null,
private maxWindowSize: number,
private scheduler: SchedulerLike) {
super(destination);
const window = this.openWindow();
if (windowCreationInterval !== null && windowCreationInterval >= 0) {
const closeState: CloseState<T> = { subscriber: this, window, context: <any>null };
const creationState: CreationState<T> = { windowTimeSpan, windowCreationInterval, subscriber: this, scheduler };
this.add(scheduler.schedule<CloseState<T>>(dispatchWindowClose, windowTimeSpan, closeState));
this.add(scheduler.schedule<CreationState<T>>(dispatchWindowCreation, windowCreationInterval, creationState));
} else {
const timeSpanOnlyState: TimeSpanOnlyState<T> = { subscriber: this, window, windowTimeSpan };
this.add(scheduler.schedule<TimeSpanOnlyState<T>>(dispatchWindowTimeSpanOnly, windowTimeSpan, timeSpanOnlyState));
}
}

IMO, all operator subscriptions made within an operator's call method should be made using subscribeAndChainOperator.

ATM, this PR's branch is based on #5239, so it includes that PR's commits. The last two commits are the changes introduced in this PR.

Related issue (if exists): #5239

@cartant cartant added AGENDA ITEM Flagged for discussion at core team meetings target: master & patch labels Jan 18, 2020
@cartant cartant removed the AGENDA ITEM Flagged for discussion at core team meetings label Apr 9, 2020
@cartant
Copy link
Collaborator Author

cartant commented Apr 22, 2020

Closed by #5333

@cartant cartant closed this Apr 22, 2020
@cartant cartant deleted the subscribe-and-chain-operator branch September 24, 2020 07:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant