48

I build a simple confirmation dialog service (Angular 2) with this method:

confirm(body?: string, title?: string): Subject<void> {
    this.confirmation = new Subject<void>();
    // ... show dialog here... "are you sure?"
    return this.confirmation;
}

_onYesClicked() {
  // ... closing the dialog
  this.confirmation.next();
  this.confirmation.complete();
} 

_onNoClicked() {
  // ... closing the dialog
  this.confirmation.complete();
}

Usage:

confirmationService.confirm().subscribe(() => alert("CONFIRMED"));

If someone uses the service, he gets a Subject (which is an Observable) returned and can "subscribe()" to it. The subscription is called when "yes" is clicked and therefore the confirmation was given...

Is this the correct way to do this? And more important... will the call to

this.confirmation.complete();

unsubscribe the subscribed listeners and therefore prevent any lingering references (memory leakage)?

Steven Liekens
  • 13,266
  • 8
  • 59
  • 85
Wolfgang
  • 2,188
  • 1
  • 25
  • 24

1 Answers1

67

If you want to be sure it removes all Observers you can check it for yourself in https://github.com/ReactiveX/rxjs/blob/master/src/internal/Subject.ts#L91. It calls complete() on all Observers (Observers are typically just dumb objects implementing Observer interface) and then sets this.observers.length = 0;. So the answer is yes.

Your approach is valid, it's basically the same as Angular2 regularly does with EventEmitter. Just one thing you can improve is start using asObservable() when exposing Subjects. This will hide the fact that you're using a Subject underneath and return just a regular Observable. This way you don't let your users to by accident (or from misunderstanding) try to call next() , complete() or error() on your Subject.

Regarding memory leakage, this has to be handled by RxJS, so you shouldn't worry about it and if there's a problem the authors would probably notice it before you.

Also have a look at this: Observable vs Subject and asObservable

seairth
  • 1,966
  • 15
  • 22
martin
  • 93,354
  • 25
  • 191
  • 226
  • I would also call error() instead of complete() when the user clicks No. Or I would use a Promise rather than an Observable. That's BTW what ng-bootstrap modals do. – JB Nizet Nov 06 '16 at 19:03
  • 1
    Hehehe ... that's genius, just looking at the source - that makes me certainly feel very sure that no references are kept. Also great advise on "asObservable()" - Thank you! – Wolfgang Nov 06 '16 at 19:10
  • 2
    @JBNizet I think clicking "No" doesn't necessarily mean an error but this is a question for OP not for me I guess. – martin Nov 06 '16 at 19:11
  • 10
    @JBNizet No offense but that is simply bad advice. Calling error() to indicate a valid user flow is not good design. Code errors are designed to indicate that something went technically wrong, not to indicate that a user said no in a dialog. Think of it this way: If a user clicks "no" on a dialog that asks "Do you want to blow up the world?", I'd say that's not an error. In short, errors are designed for unhandled code flows that would cause the program to BREAK, not to indicate which branch of logic to take next in a valid use case. – dudewad Aug 27 '18 at 15:58
  • Wow, what a great answer! I learned so much today because of this :) – Superole Apr 17 '20 at 13:49
  • Hey there, thanks for your explanation! I was wondering: You say that `Subject.complete()` calls `complete()` on all Observers. But where in the function does that actually happen? I can only see 2 lines and none of them is a function call. Has this maybe changed since your answer? – Spray'n'Pray Apr 29 '21 at 09:32