Automatically complete subjects

[lundmikkel]

We often use subjects within components. Ensuring that they are correctly completed, when the component is destroyed is a bit cumbersome and people often forget it.

Would it be possible to extend checkProperties add a new property that could look for subjects and complete them?

[NetanelBasal]

until-destroy/src/lib/until-destroy.ts

Line 18 in f03793a

property && isFunction(property.unsubscribe) && property.unsubscribe();

[NetanelBasal]

It's been called. What's the issue?

[lundmikkel]

Subjects are not completed, when calling unsubscribe. You should call complete instead.

I've made an example showing how the subject is not completed automatically, but subscriptions are: https://codesandbox.io/s/gallant-fast-i29fq?file=/src/app/components/subject.component.ts. Just click on the text and watch the console log when the subscription is completed, but not the subject.

[NetanelBasal]

That's related to how Rx works. When you call unsubscribe, it just does the following:

https://github.com/ReactiveX/rxjs/blob/master/src/internal/Subject.ts#L88

[NetanelBasal]

It doesn't relate to the library. You can manually call it, and you'll get the same behavior.

[lundmikkel]

until-destroy/src/lib/until-destroy.ts

Line 18 in f03793a

property && isFunction(property.unsubscribe) && property.unsubscribe();

So that line should be:
property && ((isFunction(property.unsubscribe) && property.unsubscribe()) || (isFunction(property.complete) && property.complete()));

(With extra many parentheses to make it a bit more clear)

[lundmikkel]

Well, I don't want to do it manually. That's kind of the point. Now I have to find all subjects and manually complete them in my ngOnDestroy. Why not allow the same behavior as with subscriptions?

[NetanelBasal]

I'm not sure we want this behavior. I'll leave it open for discussion.

[NetanelBasal]

@arturovt your opinion?

[NetanelBasal]

Unsubscribe !== complete

[lundmikkel]

Why would you not want a component's subjects to be completed when the component is? If you complete the subject then you wouldn't have to unsubscribe all the places that subject is used.

Unsubscribe !== complete

Not sure what you are trying to say. I'm just saying that being able to automatically complete my subjects on component destroy would be a very nice feature that would make my life a lot easier. Just like the package has made my life easier when completing subscriptions. Whether it should be part of the checkProperties functionality or done in some other way is less important to me.

[NetanelBasal]

Because it will call complete() on the observers, and it's not true. The observers are unsubscribed.

[NetanelBasal]

What I'm trying to say is that it's breaking the normal contract.

[lundmikkel]

Introducing a new property wouldn't break any contracts. You could easily add something like completeSubjectProperties that would be false by default.

[arturovt]

I'm afraid that would bring some inconsistent behavior since subjects have both unsubscribe and complete properties. Calling complete after the unsubscribe is called throws an error:

$ node

{ Subject } = require('rxjs')
subject$ = new Subject()
subject$.unsubscribe()
subject$.complete() // Uncaught Error [ObjectUnsubscribedError]: object unsubscribed

Checking that the complete property exists is also not an option:

isFunction(property, 'complete') && property.complete();

Because: I'll give you an example, in my applications there are many services that inherit Subject:

@Injectable({ providedIn: 'root' })
export class SomeMicroFrontendBus extends Subject { ... }

Thus calling complete on this property will break the application behavior. Of course we could've blacklisted this property, but I wouldn't want to blacklist this property in many components, that use this service, since it will bring the code duplication.

[BojanKogoj]

Don't add complete on destroy , we sometimes rely on having a response when that happens.

[lundmikkel]

Don't add complete on destroy , we sometimes rely on having a response when that happens.

You can add it in a backwards compatible way (by introducing a new property), so I don't see that as a problem.

[lundmikkel]

Calling complete after the unsubscribe is called throws an error.

True. But completing, then unsubscribing doesn't:

subject = new Subject();
subject.complete();
subject.unsubscribe(); // no error

You just ensure that you run them in the right order.

Thus calling complete on this property will break the application behavior. Of course we could've blacklisted this property, but I wouldn't want to blacklist this property in many components, that use this service, since it will bring the code duplication.

So can we just once and for all agree that making a new property would mean you could opt in, meaning you wouldn't have to do anything with your existing code? I would expect the blacklist to also apply in this case, of course.

[lundmikkel]

Any comments?

[arturovt]

I do think we have to wait for the community response. If the community needs this feature, then we will definitely add it.

[Ghostbird]

Bit of a late community response, but we had a bit of a problem for a while because we assumed that checkProperties would also complete Subjects. Now we add ngOnDestroy with complete() calls for every Subject. Personally I think checkProperties is confusing, because it doesn't tell me anything about it's functionality, it just describes a part of its implementation.

We only have the use case where we create a Subject on component construction and complete it with the component destruction. For us a completeSubjects would be a nice addition.