angular / angular.js

AngularJS - HTML enhanced for web apps!
https://angularjs.org
MIT License
58.85k stars 27.52k forks source link

ngAnimate#on, if animated element doesn't exists, empty jQuery / jqLite selector #15284

Open huttarichard opened 7 years ago

huttarichard commented 7 years ago

Hi there, great job guys! :)

Do you want to request a feature or report a bug? Maybe Report Bug

What is the current behavior? Im trying to use ngAnimate, but getting error on this line https://github.com/angular/angular.js/blob/master/src/ngAnimate/animateQueue.js#L192

TypeError: Illegal invocation
    at angular-animate.js:2314
    at forEach (modules.js?hash=242ac3e…:10867)
    at findCallbacks (angular-animate.js:2313)
    at angular-animate.js:2682
    at meteorInstall.node_modules.angular-animate.angular-animate.js.$animateProvider.$get.$rootScope.$watch.$rootScope.$$postDigest.$rootScope.$$postDigest.animationsEnabled (angular-animate.js:2243)
    at notifyProgress (angular-animate.js:2681)
    at angular-animate.js:2674
    at modules.js?hash=242ac3e…:16381
    at forEach (modules.js?hash=242ac3e…:10867)
    at Object._resolve (modules.js?hash=242ac3e…:16380)

It is because if you do something like this:

// `el instanceof jQuery` => true
// `el.length` => 0
var el = $(".slide"); 

$animate.on("enter", el, function(){
    console.log("should be fired")
});

later it will try to iterate over elements to find out if element contains target element to run callback, but this will result in error due to not existing element.

https://github.com/angular/angular.js/blob/master/src/ngAnimate/shared.js#L134

this function try to extract dom node element, but if element[0] not exists it will return empty array. This function can return anything.

So you provide an argument which jquery / jqLite selector, and get error. Questions is if empty jQuery selector is valid argument or not. In my case it is obvious, that element not exists. But what if you remove element before $animation.animation will be called (not really sure what is going to happen, but probably same case, isn't it?).

Is this bug? or am I just annoying?

What is the expected behavior? To get error, maybe right one? or just do nothing, not really sure :)

Which versions of Angular, and which browser / OS are affected by this issue? Did this work in previous versions of Angular? Please also test with the latest stable and snapshot (https://code.angularjs.org/snapshot/) versions. 1.5.8

gkalpak commented 7 years ago

This is a valid(-ish) issue. I mean passing an empty jqLite/jQuery collection to $animate.on() doesn;t make sense, but ideally we shouldn't throw either. Considering the .on() method is not called often, doing a better job at detecting native Nodes vs jqLite/jQuery collections doesn't have performance implications and may be worth it.

That said, theoretically, you should only pass native Nodes to $animate methods (according to the docs), although $animate is clearly designed to work with jqLite/jQuery collections as well. (Related https://github.com/angular/angular.js/issues/15081.)

So, if anyone wants to work on this, I would be happy to review (or help along the way), but it is a low-priority issue imo.