angular / angular.js

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

Make ngClick only $apply when it doesn't return a promise #8286

Open pocesar opened 10 years ago

pocesar commented 10 years ago

ngClick should not ignore the return value of the underlaying function. Check if it's a promise, then don't call an unnecessary $scope.$apply, since it will be resolved and applied when $q resolves.

My case: I use a third party promise library bluebird, and do a lot of UI display calculations (prices and discounts) before displaying on the UI. Currently there's a flicker on the UI because the model is emptied (while the ngClick happened) and recalculated through the bluebird promise, that is wrapped using $q.when (that also calls a second $apply when it's resolved), since it's called on the next cycle.

IgorMinar commented 10 years ago

I don't think that this has anything to do with ngClick. Rather, the Scope#$evalAsync should make it possible to schedule $q next tick via microtasks.

IgorMinar commented 10 years ago

edited the subject. original was: "Make ngClick only $apply when it doesn't return a promise"

pocesar commented 10 years ago

because clickHandler is wrapped inside scope.$apply (in https://github.com/angular/angular.js/blob/36831eccd1da37c089f2141a2c073a6db69f3e1d/src/ngTouch/directive/ngClick.js#L277)

basically this:

    element.on('click', function(event, touchend) {
      scope.$apply(function() {
        clickHandler(scope, {$event: (touchend || event)});
      });
    });

would become

    element.on('click', function(event, touchend) {
      var ret = clickHandler(scope, {$event: (touchend || event)});
      if (ret && angular.isFunction(ret.then)) {
        ret.then(scope.$digest.bind(scope)); // then should $digest only if it's a non-trusted promise (other than $q)
      } else {
        scope.$digest();
      } 
    });

(of course I'm over simplifying the issue)

bluebird by default (and as per spec) call the promise on the next tick always, for a clean stack trace. $evalAsync wouldn't make a difference in this case.

jeffbcross commented 10 years ago

@IgorMinar did you mean to edit the subject? The current subject matches what your edit says the original was.

lgalfaso commented 9 years ago

I do not understand this request. Why a function that is called inside ngClick/$eval/$evalAsync cannot have side-effects that should be propagated immediately and return a promise?

shahata commented 9 years ago

@lgalfaso the promise thing is just a confusing implementation detail (which is not even necessary for the implementation). This issue is simply about the fact that the function inside ngClick or any other event handler might not have immediate side effects and therefore it would like to somehow skip the unnecessary digest. See the suggested PR (#8301) for more details...

lgalfaso commented 9 years ago

8301 looks like an overkill If we only want to tackle this case. We should

just have new directives that eval the expression outside an $apply

shahata commented 9 years ago
  1. New directives sounds like duplicate code and more of an overkill to me...
  2. There are also ppl requesting to do local digest on the current scope instead of a full digest, would you give them a third set of directives?
lgalfaso commented 9 years ago

I am moving the conversation to #8301