fix($rootScope): don't allow explicit digest calls to affect $evalAsync

[dcherman]

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

fix

What is the current behavior? (You can also link to an open issue here)

#15127

What is the new behavior (if this is a feature change)?

If an $evalAsync call occurs prior to a $digest being invoked on a scope other than the $rootScope, the target of the operation is changed to the $rootScope in order to allow the watchers that may have reacted to the $evalAsync operation to occur

Does this PR introduce a breaking change?

Nope

Please check if the PR fulfills these requirements

• The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

Other information:

Fixes #15127

[dcherman]

@gkalpak So after thinking this over for a bit, I don't think having internal $evalAsync calls here that affect external services/scopes not be evaluated is a problem. That would be no different than a $watch affecting something similar, but not being reflected; the developer is explicitly telling us via $digest that only that subtree will be modified.

This fix does address the case of either $evalAsync (or the same via $http) being invoked prior to the $digest being called which is what the original issue was about.

[gkalpak]

I personally am fine with such a change (as I am not a strong believer in the local $digest() optimization 😃). But this will make the optimization void whenever an async task is queued - keep in mind that the following "things" use $evalAsync:

• $q (and everything that uses $q under the hood, e.g. $timeout, $http, some animation features, async ngModel validators etc)
• $interval
• $watchCollection
• ngFocus/ngBlur
• (Other internal stuff that are less likely to affect apps.)

Yet, imo voiding an optimization is preferrable to unexpectedly broken behavior (because some bindings did not propagate).

I am a little on the fence on whether this is a breaking change though, as it does break certain assumptions that used to hold. (Although it is not very likely to actually breaks apps, I wonder how tests could be affected by this.)

tl;dr

Looks OK for 1.7.x - Still not sure about 1.6.x

[petebacondarwin]

I am not that keen on this change.
I am sure that we have always encouraged people to use $apply not $digest, and those that choose not to have to accept that they are not going to get guaranteed behaviour across the app.

[gkalpak]

The problem with the current implementation is that a global $digest might not be run, because some other unrelated scope happened to also $digest locally. For example:

// Imagine two totally unrelated scopes (e.g. two directives scopes coming from
// third party libraries and located in distinct and unrelated parts of the page.
var scope1 = $rootScope.$new();
var scope2 = $rootScope.$new();

// `scope1` calls `$evalAsync()` (and expects the updated values to propagate to the view
scope1.$evalAsync(someExpr);

// Before the next tick (when `$rootScope.$digest()` will be called)
// `scope2` calls `$digest()` to update some value in its own view
scope2.$digest();

// Now, because of that last call, `someExpr` will be evaluated, the `asyncQueue` will be
// emptied and as a result, `$rootScope.$digest()` won't run.

[petebacondarwin]

Yes, and I am saying that people shouldn't be doing scope2.$digest() :-P
Unfortunately I appreciate that they do for performance reasons.

So how about we do not drain the async queue if the call to $digest was not on the root scope? This is subtly different to the current proposed solution.

[dcherman]

@petebacondarwin That would be a lot worse than the current behavior. You would no longer be able to use anything that relies on $evalAsync whether it be direct usage, an already resolved promise using .then, anything that uses $watchGroup, etc...

Even worse, $templateRequest uses $evalAsync under the hood, so this would break any components that use templateUrl if we don't drain that queue.

[gkalpak]

How about #15501? It should fix the issue with minimal impact on the current behavior (and the optimization).

[dcherman]

That's definitely an alternative, but I'm unclear on why that's better since it causes additional loops of the digest cycle whereas this keeps it as minimal as possible.

Is it to address situations where someone is mutating global (or just outside state) inside a digest cycle that was initated with $digest?

[gkalpak]

I am not sure it is better. It is just an alternative, I guess 😃

Initially I thought it was a good idea, because this keeps the execution order closer to what I would expect and it might also be less expensive if the local digest doesn't stabilize immediately. But today it doesn't seem any better than your version (both having pros and cons).

[gkalpak]

After thinking it some more, I think this one is better. So, I am again back to:

Looks OK for 1.7.x - Still not sure about 1.6.x