Deprecate SubscriptionDisposable

[Nateowami]

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.21%. Comparing base (5e0b243) to head (c413b4d).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2788      +/-   ##
==========================================
- Coverage   79.21%   79.21%   -0.01%     
==========================================
  Files         533      533              
  Lines       30974    30974              
  Branches     5053     5031      -22     
==========================================
- Hits        24536    24535       -1     
- Misses       5654     5666      +12     
+ Partials      784      773      -11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[RaymondLuong3]

Could we not update the subscription disposable class? Using the DestroyRef makes sense, but having the subscription handling logic in one superclass also makes sense. In SubscriptionDisposible, instead of listening to ngOnDestroy, we can listen to destroyRef.onDestroy. What did you have in mind?

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Nateowami)

[marksvc]

@Nateowami , I don't see examples in our codebase of how we might go about this.

It looks like we might do this by (1) injecting DestroyRef in a constructor:

constructor(
    ...
    private readonly destroyRef: DestroyRef,

And then (2) registering a Subscription's unsubscribe() method with DestroyRef.onDestroy() like so:

const subscription = this.questionDoc.remoteChanges$.subscribe(() => {});
this.destroyRef.onDestroy(() \=> {
    subscription.unsubscribe();
});

Is this what you are thinking regarding using DestroyRef.onDestroy to unsubscribe?

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Nateowami)

[Nateowami]

@marksvc Search for takeUntilDestroyed(this.destroyRef)

[marksvc]

@Nateowami Thank you. Okay, that doesn't look too bad. So in my code above, it looks like my second part would instead be written as

this.questionDoc.remoteChanges$
    .pipe(takeUntilDestroyed(this.destroyRef))
    .subscribe(() => {});

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Nateowami)

[marksvc]

Hey guys. Thanks for your consideration of what is going to be a good design for the system. @Nateowami , when you have a moment, I think the conversation looks to be waiting for you to respond to a question from @RaymondLuong3 .

I haven't researched enough on the topic to be sure, but if Angular+RxJS is moving to using .pipe(takeUntilDestroyed(this.destroyRef)) as a normal way of causing an unsubscribe when a component/etc is destroyed, I would be interested in using that normal practice in our codebase as well (at least for newly written code).

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Nateowami)

[Nateowami]

Actually, I just looked at the documentation for takeUntilDestroyed, and the DestroyRef is optional, as it can automatically inject it in most contexts:

/**
 * Operator which completes the Observable when the calling context (component, directive, service,
 * etc) is destroyed.
 *
 * @param destroyRef optionally, the `DestroyRef` representing the current context. This can be
 *     passed explicitly to use `takeUntilDestroyed` outside of an [injection
 * context](guide/dependency-injection-context). Otherwise, the current `DestroyRef` is injected.
 *
 * @developerPreview
 */
export declare function takeUntilDestroyed<T>(destroyRef?: DestroyRef): MonoTypeOperatorFunction<T>;

I also think that there are serious disadvantages to the current approach. Inheritance is often ugly and can cause issues. See #566 and #2754, two separate instances where I've had to fix it not being implemented correctly.

[Nateowami]

Actually, I think I somewhat misunderstood the documentation. If you try to remove it, you end up with this error:

NG0203: takeUntilDestroyed() can only be used within an injection context such as a constructor, a factory function, a field initializer, or a function used with runInInjectionContext. Find more at https://angular.io/errors/NG0203

So in most cases you really do need to pass a DestroyRef.

[marksvc]

I tried removing it from a couple places yesterday and got an error as well. I was hoping we could simplify it that much further, as well :)

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Nateowami)

[Nateowami]

@RaymondLuong3 That would require injecting the DestroyRef into every component, and then passing it to the parent class using a super(destroyRef) call. I generally prefer to avoid inheritance as much as possible, as it can cause a lot of added complexity, and I don't see it being any simpler to implement in the super class than in each individual class.

[RaymondLuong3]

Yes, that is correct. We would have to inject destroyRef into every component that is a subclass of SubscriptionDisposable. I was thinking that once we set that up the first time, then in each subsequent subclass, injecting destroyRef and providing it to the parent class would be straight forward and the parent class can handle the takeUntilDestroyed pipe. Both seem a bit cumbersome, but I see the advantage of not having the super class overhead. I will approve. Should we make a card to remove subscription disposable?

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Nateowami)

[Nateowami]

Should we make a card to remove subscription disposable?

Maybe? I don't think we need to at this time; over time it will have fewer users, and eventually we can rip out out entirely. I would focus on other things that are deprecated first (like RxJS toPromise) that we don't control.