search

flauc / angular2-notifications

A light and easy to use notifications library for Angular.
https://stackblitz.com/edit/angular2-notifications-example
MIT License
746 stars 164 forks source link

refactor: change signature of NotificationType interface #288

Open jotatoledo opened 6 years ago

jotatoledo commented 6 years ago

Current behavior

ATM the NotificationTypeinterface 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 Observablebehavior of the streams.

Expected behavior

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

flauc commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 NotificationServiceusing a Notificationinstance, as it wouldn't expose the next method. A workaround would be to hard cast the Observableto Subjectand 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:

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 commented 6 years ago

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?