search

cgross / angular-busy

Show busy/loading indicators on any element during $http requests (or any promise).
http://cgross.github.io/angular-busy/demo
MIT License
1.44k stars 256 forks source link

Fix angular-busy promise-compare issues and added UT #85

Open benbracha opened 8 years ago

benbracha commented 8 years ago

Related also to issue https://github.com/cgross/angular-busy/issues/68

In case I change cg-busy unresolved promise with a new unresolved promise - the tracker.reset is not begin called. This is due the use of angular.equals, which doesn't look on "$" fields - so the two above promises looks the same. Because reset is not being called, when the first promise is resolved - the cg-busy overlay is removed, which causes unwanted behaviour (the user still waits for an action, but no overlay).

Therefore, we add to the promise object a random ID, so comparison works correctly. Added also UT that catch this bug, and works after our fix.

Thanks to @barnash for the help in debugging and finding a solution for it.

benbracha commented 8 years ago

Looking at the angular-busy code again, actually - I'm not sure what is the need for angular.compare on the promises at all. Meaning - removing my fix and the compare check together, will have the same result (and fix the issue I originally tried to fix).

All the logic is triggered under $watchCollection on the options parameter. Two scenarios I see here:

  1. The options is a promise, or array of promises. In this case - if the $watchCollection was triggered, the angular.equal will always return true because there is an actual change of the promises.
  2. The options is an object, with promise or array of promises supplied. In this case - if the $watchCollection was triggered it is either the promise was changed or other key in the object (like the min-duration or templateUrl key). If it was a promise change - we need to call reset anyway. If not - calling reset again seems not to have any effect. It may create more callbacks for delay/min-duration which is a waste, but it doesn't seem to have any effect.

To be on the safe side - I'll keep my fix because if fixes the author original intention - trigger reset only on promise changes..

faceleg commented 8 years ago

I've forked this to https://github.com/faceleg/angular-busy-plus. Recommend using that as the development focus until / unless the owner becomes active or brings help on board.