Skip to content

feat(rule): do not check for the complete method to be called since i…#3

Merged
macjohnny merged 1 commit intomasterfrom
feature/removeCompleteCheck
Mar 12, 2020
Merged

feat(rule): do not check for the complete method to be called since i…#3
macjohnny merged 1 commit intomasterfrom
feature/removeCompleteCheck

Conversation

@nivekcode
Copy link
Copy Markdown
Member

@nivekcode nivekcode commented Mar 12, 2020

On Twitter people pointed out that we don't need to call the complete method of the destroy$.

I just had a look into the RxJS source code and saw that take until doesn't only unsubscribe from the source but also from the notifier.

  constructor(private notifier: Observable<any>) {
  }

  call(subscriber: Subscriber<T>, source: any): TeardownLogic {
    const takeUntilSubscriber = new TakeUntilSubscriber(subscriber);
    const notifierSubscription = subscribeToResult(takeUntilSubscriber, this.notifier);
    if (notifierSubscription && !takeUntilSubscriber.seenValue) {
      takeUntilSubscriber.add(notifierSubscription);
      return source.subscribe(takeUntilSubscriber);
    }
    return takeUntilSubscriber;
  }
}

Not only the source but also the notifier gets unsubscribed. So, therefore, we don't need to call complete. @macjohnny, @tomastrajan what do you think?

@macjohnny
Copy link
Copy Markdown
Member

@kreuzerk good point, I think you are right. Some while ago I tested the effect of omitting the this.destroy$.complete() call, and there were no excessive Subscribers in the heap, but since this pattern is also used e.g. in
https://github.com/angular/components/blob/0ed9416ab9690be219eab799042777f31e5df823/src/cdk/layout/breakpoints-observer.ts#L52-L56
I was too afraid to remove it ;-)

So I think we should include your change.

@nivekcode
Copy link
Copy Markdown
Member Author

Here's some further information about the topic:
https://twitter.com/Michael_Hladky/status/1193641068635590656

Copy link
Copy Markdown
Member

@macjohnny macjohnny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@macjohnny macjohnny merged commit d661791 into master Mar 12, 2020
@macjohnny macjohnny deleted the feature/removeCompleteCheck branch March 12, 2020 07:22
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.

2 participants