fix(upgrade): call setInterval outside the Angular zone #16836
Conversation
| const subscription = | ||
| this.ngZone.onMicrotaskEmpty.subscribe(() => $rootScope.$digest()); | ||
| const subscription = this.ngZone.onMicrotaskEmpty.subscribe(() => { | ||
| if (!$rootScope.$$phase) { |
There was a problem hiding this comment.
This is something we've been trying to avoid 😃
It can hide actual problems in user code. It is better to identify the cause of this and stop it.
There was a problem hiding this comment.
In this case, the cause is that $interval runs the callback with evalAsync then triggers $rootScope.$apply. If we call ngZone.run() within this callback, it looks like that can synchronously trigger onMicrotaskEmpty, which also calls $rootScope.$digest.
Using $$phase is ugly here, though, I agree. The alternative is to put ngZone run in a setTimeout() so it'll run on the next vm turn (and thus, outside of the digest started by $interval). Either way is fine with me, or if there's a third option I haven't considered, let me know :)
|
|
||
| tick(200); | ||
| expect(intervalDone).toEqual(true); | ||
| $interval.cancel(id); |
There was a problem hiding this comment.
I would verify that the returned value (id) is as expected and that .cancel() works (just to be sure).
There was a problem hiding this comment.
The fakeAsync zone will throw an error if the test ends while there are pending tasks, but I put in some expectations around cancelling.
heathkit
force-pushed
the
interval-fix
branch
3 times, most recently
from
45004db
to
55d2df1
Compare
heathkit
changed the title
| (intervalDelegate: angular.IIntervalService) => { | ||
| // Wrap the $interval service so that setInterval is called outside NgZone, | ||
| // but the callback is still invoked within it. This is so that $interval | ||
| // won't block stability, which was the case in AngularJS. |
There was a problem hiding this comment.
Suggestion: "which preserves the behavior from AngularJS"
This wraps the $interval service when using upgrade to run the $interval() call outside the Angular zone. However, the callback is invoked within the Angular zone, so changes still propagate to downgraded components.
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
This wraps the $interval service when using upgrade to run the
$interval() call outside the Angular zone. However, the callback is
invoked within the Angular zone, so changes still propagate to
downgraded components. Fixes #16349.
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behavior? (You can also link to an open issue here)
In AngularJS, the
$intervalservice wraps setTimeout so that$digestwill be called after the interval fires. Because$intervaldoes not block stability in AngularJS, we recommended that Protractor users use it instead of$timeoutfor long running or polling tasks.Upon upgrading, those user's tests start failing, because the
$intervalservice will block stability for the Angular zone, whereas it didn't in AngularJS.What is the new behavior?
This wraps the
$intervalservice so thatsetTimeoutis called outside the Angular zone. However, the callback is still run within the Angular zone, so that changes still propagate (ie, this does not address #6385 at all).Does this PR introduce a breaking change? (check one with "x")
I think it technically might be a breaking change, but the previous behavior was not what people would expect when upgrading from AngularJS.