search

scttcper / ngx-toastr

🍞 Angular Toastr
https://ngx-toastr.vercel.app
MIT License
2.51k stars 359 forks source link

is it really smart to use provideIn:root for the toaster service? #896

Open jcompagner opened 3 years ago

jcompagner commented 3 years ago

If we use it in our module and we use forRoot:

https://github.com/Servoy/webnotifications/blob/73b9665b170e025dee603f46ec5214d511b1463e/webnotifications/projects/webnotifications/src/webnotifications.module.ts#L15

then it still doesn't work to if we inject the ToastrService into one of our own (ServoyToastrService which is provided by the above module)

We always will get NullInjector for TOAST_CONFIG, why that exactly is i don't know i always wonder what that forRoot() does when including a module, (and we also use Router further up the tree)

Also in our product we tried to use provideIn:root for a lot of service but many are already reverted back, because of these kind of problems, its very tricky to get it all right then, If a service is provided in root, then everything it also include must suddenly also be in root. and that is not always that easy todo. Besides the problem that they say provideIn root is to make sure it is a singleton, we never had any problems that it was not a singleton UNTIL we start using provideIn:root then suddenly we had problems of multiply instances (because we forgot somewhere to also not include it in a module)

To fix toaster is to https://github.com/Servoy/webnotifications/blob/73b9665b170e025dee603f46ec5214d511b1463e/webnotifications/projects/webnotifications/src/webnotifications.module.ts#L11

provide the ToastrService by our own module, which is a bit funny code if you asked me, but i don't see any other way to fix this

So i really thing you should not do provide in root but really provide it by the Toaster Module, so anybody that just includes that gets a config and the toaster service, that is so much more simple to understand...

scttcper commented 3 years ago

are you importing webnotifications module at the root of your app?

jcompagner commented 3 years ago

no because that is not possible, the root of the app has no idea.. this module can be optionally included further down the tree. The root app should not have to know this .

scttcper commented 3 years ago

That's fine but if you try to limit the number of notifications shown at a time or clear all notifications you'll likely run into issues since you aren't guaranteed a singleton