catdadcode / angular-spinners

A library for easily managing loading spinners in complex Angular2+ applications.
MIT License
140 stars 49 forks source link

One global spinner #10

Closed tamitutor closed 8 years ago

tamitutor commented 8 years ago

I've embedded a "spinner" element in the top level view with the idea that it will be used by all child views and their controllers...However, when transitioning to another view/controller I'm getting the error "A spinner with the name 'loadingSpinner' has already been registered." Is there a way to have one spinner in a parent view/controller?

(I'm using ui-router and for reasons not applicable to this discussion I'm reloading the view and it's controller.)

catdadcode commented 8 years ago

There is an undocumented method on the spinnerService specifically for this edge-case. Put "spinnerService._unregister('spinner name');" wherever is appropriate for your app. In your case I would say just as your views are transitioning (there is likely an emitted angular event you can capture if needed). This will unregister the spinner with the service and allow the directive to re-register itself the next time it's parsed by angular.

There is also a _unregisterGroup if you have a group of spinners that need to be unregistered with each view change.

I hope that helps.

catdadcode commented 8 years ago

Scratch that. I found a better way! Apparently there is a $destroy event that is emitted when a scope is destroyed, which happens when ui-router destroys directives inside a view it's removing. In other words, the directive now makes its own _unregister call so you don't have to!

This is available in version 3.0+ which I just barely published ;)

tamitutor commented 8 years ago

I went ahead and changed my version of the code so that if the spinner is already is registered it doesn't throw an error and just skips adding it.

tamitutor commented 8 years ago

I posted that last comment before I saw your resolution. I updated to the latest (3.0.1 as of this writing) and everything works great with ui-router. Thanks!

catdadcode commented 8 years ago

No worries. Glad you got it working :)

Just so you know, your solution of skipping re-adding it would actually break the new spinner being created because the api object registered with the spinnerService would be the old object provided by the old (now destroyed) spinner, and the new spinner would not have registered a new API object so that the service could properly control it.

eliseocastilla commented 8 years ago

This issue is happening again with ui-router 0.2.16. It was working just fine with 0.2.15

catdadcode commented 8 years ago

It appears there's not much we can do. They changed the way things work and I'm not sure how to work around it anymore.

https://github.com/angular-ui/ui-router/issues/2485

We might have to go back to the previous manual solution for now :(


There is an undocumented method on the spinnerService specifically for this edge-case. Put "spinnerService._unregister('spinner name');" wherever is appropriate for your app. In your case I would say just as your views are transitioning (there is likely an emitted angular event you can capture if needed). This will unregister the spinner with the service and allow the directive to re-register itself the next time it's parsed by angular.

There is also a _unregisterGroup if you have a group of spinners that need to be unregistered with each view change.

I hope that helps.

catdadcode commented 8 years ago

The issue is that the $destroy event is delayed now due to animate stuff or something according to the issue I linked above. The new spinner directive in the new view is trying to register itself with the spinner service before the $destroy event is emitted to trigger the old directive to unregister itself.

The only solution I can think of is to go with a "last directive to register wins" approach so that new directives trying to register with the same ID won't throw an error but will instead override the old tracked directive. I don't like it, but unless anyone has a better idea I think it's all we can do to avoid every ui-router user having to manually unregister their spinner directives from the spinner service.

catdadcode commented 8 years ago

Okay. After thinking about it I decided that depending on the ordering of the $destroy event is just not going to be reliable. Likewise, listening for ui-router's $stateChangeStart event is a dirty hack IMO because this library shouldn't have to be aware of specific events from another library. Given these points I've decided the "last one wins" registration method for spinner directives is just going to be easier and cause fewer headaches.

The one caveat to this is that now if users try to register spinners with the same name or have duplicate spinners by some other fluke, the last spinner to register with the same name will be the only one tracked by the service. This means that if the spinner being overridden in the spinner service wasn't actually destroyed from the DOM you could potentially end up with a dead spinner that you can't interact with via the service. That said, this pattern is probably better because this issue isn't the fault of the library but the application consuming it. The error message was an attempt to prevent mistakes but I think it introduces too many restrictions and conflicts.

https://github.com/codetunnel/angular-spinners/commit/7ab18bb27df112690821b84d0e776e01aac50ed9

Upgrade to 3.1.1 to receive the changes. Thanks guys.

eliseocastilla commented 8 years ago

Thank you very much for your quick fix!