UpgradeModule: Exception in $digest breaks change propagation #24680
Comments
|
please, add repo for reproduce |
jasonaden
added
state: Follow Up
needs reproduction
|
I'm sorry but reported issues require a reproduction of the issue. We suggest using StackBlitz, Plunker or Github. If this issue persists, please create a reproduction using one of the links above. Describe the difference between the expected and current behavior in a new issue with the reproduction linked. |
|
For |
|
Thank you for the starting point, I just spent the whole day setting up a demo but I just cannot repro the issue. My temporary solution is to blacklist focus and blur in zone.js:
But:I still think the code within the subscription is just wrong. An exception within $digest should not leave UpgradeModule in a faulty state. |
|
Generally, Now that you've identified the source of the issue, are you able to create a reproduction? |
|
Here it is: https://ngupgradestatic-playground-26mk87.stackblitz.io/ Counter stops refreshing after button is clicked. It is somehow related to Do I need to open another issue? |
|
Interesting usecase 😕 |
gkalpak
added
freq1: low
target: patch
|
I think this is an issue that
Then To resolve the issue now, you can add the following lines after __zone_symbol__BLACK_LISTED_EVENTS = ['transitionend', 'focus', 'pointermove', 'mousemove', 'pointerdown', 'mouseout', 'blur'];The Updated, @steckerl , oh, you have found the solution, I didn't read your comment.here is a working sample, https://stackblitz.com/edit/ngupgradestatic-playground-rbww6n?file=index.html @gkalpak, I think in the most cases, those |
|
Thx for looking into this @JiaLiPassion. As far as I understand, this is a general Angular Material consideration, not specific to ngUpgrade, right? If so, maybe the Angular Material docs are a more suitable place. Maybe you could open an issue or submit a PR on their repo. (As far as ngUpgrade is concerned, I think the user should not have to worry about these things causing "digest in progress" errors, so I am still in favor of using a "safe apply" |
|
@gkalpak, yes, in fact, I am also wondering this is not an I have a question, in this case, should we use // angular1 component
elem.addEventListener('click', clickHandler);
elem.addEventListener('focus', focusHandler);
const clickHandler = function() {
elem.focus();
};
const focusHandler = function() {};such code in |
|
With For the same reasons, we call |
|
@gkalpak, thank you, that makes sense.
Such kind of single direction thing will work as expected. And in this case.
So |
Yes, but I guess it is not easy to know what really triggered the |
|
@gkalpak, maybe we can create a special ngZone.onMicrotaskEmpty.subscribe(() => {
// already in digestZone, just return
if (Zone.current.get('digestZone')) {
return;
}
digestZone.run(() => {
$digest();
});
});I will write a sample to test it. |
|
The same problem happens with upgraded components that use angular-ui components, so it's not only angular-material. Same problem, |
|
I ran into this problem as well. I tracked down the source of the |
|
Wrapping |
|
Sounds good I'll try to get a PR ready tomorrow! |
|
Sorry I haven't gotten to this yet! Hope to do it soon |
|
Hey here, I am having this issue too and have submitted a PR #28337 to catch the exception as suggested by @gkalpak I am not sure what to do with the error in the Also, I was stuck with the test, any help how to write the test for this will be appreciated |
gkalpak
added
area: upgrade
|
@gkalpak I've looked into a fix. I tried to recreate the issue, but i cannot create a minimal steps to reproduce the bug. It seems to be something to do with running a I do not think that checking the I have 2 places in my app where thats happening and both places are to do with I've created a build with the If some can create repro steps with minimal code and no 3rd party libraries, I will create a PR. |
|
I'm also running into this issue. A third-party AngularJS component (ui-grid) is handling $document.on('mouseup', handler) and then calling $timeout() from the handler, which runs outside the Angular zone because it's a jQuery event, and results in broken change detection. Looks like a lot of people are actively working on the PR, but I'd be happy to lend a hand if needed. I've confirmed that the proposed changes do fix my issue locally. |
|
@MikeMatusz I've kinda been stuck on trying to make "real life" case for the tests, because I believe that If you test the implementation that test case is useless |
|
@dblVs I'll take a look at your StackBlitz today. Also, in my case, removing ngAnimate doesn't make a difference, so I don't think it's exclusive to that, though it may be a common case. @gkalpak What can we do from AngularJS code to ensure $digests are called from the Angular zone? As a temporary workaround, I'm downgrading NgZone and calling .run() in the couple places that are causing problems for me, but it seems like a flaw in UpgradeModule for that to be necessary. Would it make sense for $scope.$apply and $timeout to be patched to ensure they're running in the Angular zone if they're called from outside of it? |
|
@MikeMatusz, I don't think it matters. Why do you want to always run In any case, if anyone is looking for a (quick and dirty) work around for this issue, you could monkeypatch angular.
module('myApp').
decorator('$rootScope', $delegate => {
const originalDigest = $delegate.$digest;
$delegate.$digest = () => {
if (!$delegate.$$phase) {
originalDigest.call($delegate);
}
};
return $delegate;
}); |
|
@gkalpak Based on the zone discussion at the top of this thread, and the comments on #18979 which resulted in the same error, it seems like a reasonable question. The guidance on 18979 was to make sure that AngularJS was bootstrapped in the NgZone. When $digest is called in the NgZone, I don't think this error is possible, so I was just wondering if there was a proactive approach possible to preventing the error instead of just try/catch/continue. |
|
The error is possible even when bootstrapping AngularJS inside the Angular zone. So, ensuring the bootstrapping happens inside the Angular zone does not eliminate the error. |
…is in progress Fixes angular#24680, where onMicrotaskEmpty was triggering an extra $digest in both dynamic UpgradeAdapter and static UpgradeModule.
…st` is in progress Previously, under certain circumstances, `NgZone#onMicrotaskEmpty` could emit while a `$digest` was in progress, thus triggering another `$digest`, which in turn would throw a `$digest already in progress` error. Furthermore, throwing an error from inside the `onMicrotaskEmpty` subscription would result in unsubscribing and stop triggering further `$digest`s, when `onMicrotaskEmpty` emitted. Usually, emitting while a `$digest` was already in progress was a result of unintentionally running some part of AngularJS outside the Angular zone, but there are valid (if rare) usecases where this can happen (see angular#24680 for details). This commit addresses the issue as follows: - If a `$digest` is in progress when `onMicrotaskEmpty` emits, do not trigger another `$digest` (to avoid the error). `$evalAsync()` is used instead, to ensure that the bindings are evaluated at least once more. - Since there is still a high probability that the situation is a result of programming error (i.e. some AngularJS part running outside the Angular Zone), a warning will be logged, but only if the app is in [dev mode][1]. [1]: https://github.com/angular/angular/blob/78146c189/packages/core/src/util/ng_dev_mode.ts#L12 Fixes angular#24680
…st` is in progress (angular#29794) Previously, under certain circumstances, `NgZone#onMicrotaskEmpty` could emit while a `$digest` was in progress, thus triggering another `$digest`, which in turn would throw a `$digest already in progress` error. Furthermore, throwing an error from inside the `onMicrotaskEmpty` subscription would result in unsubscribing and stop triggering further `$digest`s, when `onMicrotaskEmpty` emitted. Usually, emitting while a `$digest` was already in progress was a result of unintentionally running some part of AngularJS outside the Angular zone, but there are valid (if rare) usecases where this can happen (see angular#24680 for details). This commit addresses the issue as follows: - If a `$digest` is in progress when `onMicrotaskEmpty` emits, do not trigger another `$digest` (to avoid the error). `$evalAsync()` is used instead, to ensure that the bindings are evaluated at least once more. - Since there is still a high probability that the situation is a result of programming error (i.e. some AngularJS part running outside the Angular Zone), a warning will be logged, but only if the app is in [dev mode][1]. [1]: https://github.com/angular/angular/blob/78146c189/packages/core/src/util/ng_dev_mode.ts#L12 Fixes angular#24680 PR Close angular#29794
…st` is in progress (#29794) (#30107) Previously, under certain circumstances, `NgZone#onMicrotaskEmpty` could emit while a `$digest` was in progress, thus triggering another `$digest`, which in turn would throw a `$digest already in progress` error. Furthermore, throwing an error from inside the `onMicrotaskEmpty` subscription would result in unsubscribing and stop triggering further `$digest`s, when `onMicrotaskEmpty` emitted. Usually, emitting while a `$digest` was already in progress was a result of unintentionally running some part of AngularJS outside the Angular zone, but there are valid (if rare) usecases where this can happen (see #24680 for details). This commit addresses the issue as follows: - If a `$digest` is in progress when `onMicrotaskEmpty` emits, do not trigger another `$digest` (to avoid the error). `$evalAsync()` is used instead, to ensure that the bindings are evaluated at least once more. - Since there is still a high probability that the situation is a result of programming error (i.e. some AngularJS part running outside the Angular Zone), a warning will be logged, but only if the app is in [dev mode][1]. [1]: https://github.com/angular/angular/blob/78146c189/packages/core/src/util/ng_dev_mode.ts#L12 Fixes #24680 PR Close #29794 PR Close #30107
…st` is in progress (angular#29794) Previously, under certain circumstances, `NgZone#onMicrotaskEmpty` could emit while a `$digest` was in progress, thus triggering another `$digest`, which in turn would throw a `$digest already in progress` error. Furthermore, throwing an error from inside the `onMicrotaskEmpty` subscription would result in unsubscribing and stop triggering further `$digest`s, when `onMicrotaskEmpty` emitted. Usually, emitting while a `$digest` was already in progress was a result of unintentionally running some part of AngularJS outside the Angular zone, but there are valid (if rare) usecases where this can happen (see angular#24680 for details). This commit addresses the issue as follows: - If a `$digest` is in progress when `onMicrotaskEmpty` emits, do not trigger another `$digest` (to avoid the error). `$evalAsync()` is used instead, to ensure that the bindings are evaluated at least once more. - Since there is still a high probability that the situation is a result of programming error (i.e. some AngularJS part running outside the Angular Zone), a warning will be logged, but only if the app is in [dev mode][1]. [1]: https://github.com/angular/angular/blob/78146c189/packages/core/src/util/ng_dev_mode.ts#L12 Fixes angular#24680 PR Close angular#29794
|
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. |
I'm submitting a...
Current behavior
When $rootScope.$digest throws an exception ($digest already in progress), change propagation stops working for downgraded components.
That is because the exception breaks the subscription on onMicrotaskEmpty here:
https://github.com/angular/angular/blob/master/packages/upgrade/src/static/upgrade_module.ts#L244
Expected behavior
UpgradeModule should be more fault tolerant (try-catch?)
or
Use a different pattern. (Check if digest already running?)
Minimal reproduction of the problem with instructions
I get this "$digest already in progress" almost every time I open an angularjs material dialog or a sidenav.
What is the motivation / use case for changing the behavior?
Environment
The text was updated successfully, but these errors were encountered: