angular / angular.js

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

Unnecessary copy on anchor animations #14622

Closed NickBolles closed 8 years ago

NickBolles commented 8 years ago

At https://github.com/angular/angular.js/blob/master/src/ngAnimate/animateCssDriver.js#L144 (for the out animation) and https://github.com/angular/angular.js/blob/master/src/ngAnimate/animateCssDriver.js#L164 (for the in animation)

$animateCss is called with the options in line, which executes the init function located here, https://github.com/angular/angular.js/blob/master/src/ngAnimate/animateCss.js#L456

This will always call copy on the options, even though they are in line and are not used again.

It seems to me that calling prepareAnimationOptions() inline at these two spots would save some operations, albeit not many, but a performance increase is a performance increase.

var animator = $animateCss(clone, prepareAnimationOptions({
   to: calculateAnchorStyles(inAnchor),
   addClass: NG_IN_ANCHOR_CLASS_NAME + ' ' + toAdd,
   removeClass: NG_OUT_ANCHOR_CLASS_NAME + ' ' + toRemove,
   delay: true
}));

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.

angular.version 1.5.5

gkalpak commented 8 years ago

I don't think that by skipping copy on these tiny, simple objects would have any impact on the overall performance, considering all the operations involved in an animation (both in JS and the DOM related ones - it even calls getBoundingClientRect() :stuck_out_tongue: ).

So, we would be duplicating the logic (by calling prepareAnimationOptions() in two places) and raising the complexity and "possible future bug surface area" for no real benefit.

I'm going to close this, but feel free to continue the discussion below.