fix($q): promises should still resolve outside of the $digest/$apply phase #3539

jamestalmage · 2013-08-10T04:38:59Z

A number of issues have been raised (see #2431) because the current promise implementation has the unexpected behavior of not forwarding execution to the onResolve or onReject callbacks until the next $digest cycle, nor does it schedule a new cycle.

The currently recommended solution is to wrap all callbacks with $apply, but that is unnecessarily cumbersome (the user will almost always wants $apply called). It becomes increasingly problematic when dealing with longer promise chains, or using third party libraries that expect conventional promises as input.

Closes #2431

…phase Add $rootScope.$apply() call upon resolve/reject if outside a digest phase Closes angular#2431

mary-poppins · 2013-08-10T04:39:02Z

Thanks for the PR!

Contributor signed CLA now or in the past
- If you just signed, leave a comment here with your real name
PR's commit messages follow the commit message format

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

jamestalmage · 2013-08-10T04:41:58Z

CLA is recently signed. Not sure what I did wrong on my commit message (I installed the hook)!

jamestalmage · 2013-08-10T04:58:49Z

src/ng/timeout.js

    var deferreds = {};

+     var $q = qFactory(function(callback){
+         $rootScope.$evalAsync(callback);
+     },$exceptionHandler);


$timeout still needs the old nextTick implementation for ngAnimate/etc unit tests to pass.

I am not sure that my hack of $timeout is the appropriate one. Perhaps the promise API should be extended to include another function skipApply() that will suppress the $apply call upon promise resolution/rejection. i.e.:

var defer = $q.defer().skipApply(); // and/or var promise = somePromiseReturningFunc(); promise.then(onResolve,onErr).skipApply();

even in the skipApply case I would think continuation of the promise chain should not wait for a $digest cycle, but simply prevent dirty checking of the scope.

IgorMinar · 2013-08-10T16:16:20Z

Thanks for PR. We are aware of this issue, but I think your solution will cause too much breakage and timing issues.

The $timeout hack is just a manifestation of this.

Also with your change, you have to use $browser in your tests, which is one of the apis that is currently not even documented as public api and our goal is to completely get rid of it for many other reasons.

jamestalmage · 2013-08-11T00:43:21Z

I am not so sure it would introduce new timing issues, except those that are desired.

Currently it has the same behavior as the old implementation unless the current evalAsync loop ends without a call to $apply. The following scenarios should all behave identically:

Already in a $apply loop.
$apply is called immediately after resolve or reject (i.e. per the documentation)
$apply is called by some another callback on the evalAsync stack.

jamestalmage · 2013-08-11T01:27:32Z

As for the $browser usage, that is trivial to replace when a more appropriate replacement is developed.

This change causes a new $digest to be scheduled in the next tick if a task was was sent to the $evalAsync queue from outside of a $digest or an $apply. While this mode of operation is not common for most of the user code, this change means that $q promises that utilze $evalAsync queue to guarantee asynchronicity of promise apis will now also resolve outside of a $digest, which turned out to be a big pain point for some developers. The implementation ensures that we don't do more work than needed and that we coalese as much work as possible into a single $digest. The use of $browser instead of setTimeout ensures that we can mock out and control the scheduling of "auto-flush", which should in theory allow all of the existing code and tests to work without negative side-effects. Closes angular#3539 Closes angular#2438

IgorMinar · 2013-08-22T09:27:28Z

please take a look at the commit above (which I threw into PR #3699)

IgorMinar · 2013-08-23T17:09:42Z

closing this one in favor of #3699

This change causes a new $digest to be scheduled in the next tick if a task was was sent to the $evalAsync queue from outside of a $digest or an $apply. While this mode of operation is not common for most of the user code, this change means that $q promises that utilze $evalAsync queue to guarantee asynchronicity of promise apis will now also resolve outside of a $digest, which turned out to be a big pain point for some developers. The implementation ensures that we don't do more work than needed and that we coalese as much work as possible into a single $digest. The use of $browser instead of setTimeout ensures that we can mock out and control the scheduling of "auto-flush", which should in theory allow all of the existing code and tests to work without negative side-effects. Closes angular#3539 Closes angular#2438

This change causes a new $digest to be scheduled in the next tick if a task was was sent to the $evalAsync queue from outside of a $digest or an $apply. While this mode of operation is not common for most of the user code, this change means that $q promises that utilze $evalAsync queue to guarantee asynchronicity of promise apis will now also resolve outside of a $digest, which turned out to be a big pain point for some developers. The implementation ensures that we don't do more work than needed and that we coalese as much work as possible into a single $digest. The use of $browser instead of setTimeout ensures that we can mock out and control the scheduling of "auto-flush", which should in theory allow all of the existing code and tests to work without negative side-effects. Closes #3539 Closes #2438

This change causes a new $digest to be scheduled in the next tick if a task was was sent to the $evalAsync queue from outside of a $digest or an $apply. While this mode of operation is not common for most of the user code, this change means that $q promises that utilze $evalAsync queue to guarantee asynchronicity of promise apis will now also resolve outside of a $digest, which turned out to be a big pain point for some developers. The implementation ensures that we don't do more work than needed and that we coalese as much work as possible into a single $digest. The use of $browser instead of setTimeout ensures that we can mock out and control the scheduling of "auto-flush", which should in theory allow all of the existing code and tests to work without negative side-effects. Closes angular#3539 Closes angular#2438

fix($q): promises should still resolve outside of the $digest/$apply …

adeeb52

…phase Add $rootScope.$apply() call upon resolve/reject if outside a digest phase Closes angular#2431

jamestalmage reviewed Aug 10, 2013
View reviewed changes

ghost assigned IgorMinar and btford Aug 21, 2013

IgorMinar closed this Aug 23, 2013

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix($q): promises should still resolve outside of the $digest/$apply phase #3539

fix($q): promises should still resolve outside of the $digest/$apply phase #3539

jamestalmage commented Aug 10, 2013

mary-poppins commented Aug 10, 2013

jamestalmage commented Aug 10, 2013

jamestalmage Aug 10, 2013

IgorMinar commented Aug 10, 2013

jamestalmage commented Aug 11, 2013

jamestalmage commented Aug 11, 2013

IgorMinar commented Aug 22, 2013

IgorMinar commented Aug 23, 2013

fix($q): promises should still resolve outside of the $digest/$apply phase #3539

fix($q): promises should still resolve outside of the $digest/$apply phase #3539

Conversation

jamestalmage commented Aug 10, 2013

mary-poppins commented Aug 10, 2013

jamestalmage commented Aug 10, 2013

jamestalmage Aug 10, 2013

Choose a reason for hiding this comment

IgorMinar commented Aug 10, 2013

jamestalmage commented Aug 11, 2013

jamestalmage commented Aug 11, 2013

IgorMinar commented Aug 22, 2013

IgorMinar commented Aug 23, 2013