refactor: change signature of NotificationType interface

[jotatoledo]

Current behavior

ATM the NotificationType interface defines the click streams with the EventEmitter<{}> , which is IMO a bad practice as explained here . Plus exposing the observer/observable subjects at the same time is a major abstraction leak: consumers of the interface should only see the Observable behavior of the streams.

Expected behavior

Change the interface to define the click streams through Observable<MouseEvent>

[flauc]

Tha is great advice thank you @jotatoledo

I will try to update it as soon as I can. If however, you feel comfortable about it, please feel free to submit a PR.

[jotatoledo]

Yeah I can submit the pr if I find some time during the weekend. Ill change the definition from EventEmitter<{}> to Observable<void>, is that ok? I dont see the point of emiting the click events, they dont provide any relevant info IMO.

[jotatoledo]

Hey @flauc Im currently working on this. The biggest issue that I encountered until now is the following:

The current implementation required that the streams defined by the Notification interface expose both the observable and subject behaviors. This is indeed a really big abstraction leak, as the end consumer of such an instance could easily call next/emit in one of those streams and fake an event by doing it.

Sadly doing the suggested change would make impossible to emit events from the NotificationService using a Notification instance, as it wouldn't expose the next method. A workaround would be to hard cast the Observable to Subject and then invoke the method, but that would be a bad refactoring IMO.

This is partially caused by a wrong segregation of responsibilities in the Notification interface: as it acts as configuration object and reference to a generated notification. This should be IMO spitted in 2:

• A class/interface that contains a notification config, f.e NotificationOptions
• A class that references an opened notification, f.e NotificationRef

Ill do some changes in the implementation logic, using some ideas from the mat-dialog implementation and then Ill do a pull request, but it will take a while.

[flauc]

Hi @jotatoledo,

Thank you for taking the time. I'm very bad on time currently, but here is something we did, using a different approach.

https://github.com/Jaspero/ng-alerts

I'm sure it's the better approach, but never had the time to refactor. Do you think it might be worth doing now?