-
Notifications
You must be signed in to change notification settings - Fork 26.2k
fix(upgrade): do not break if onMicrotaskEmpty
emits while a $digest
is in progress
#29794
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
f00be67
to
ca8b983
Compare
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the ℹ️ Googlers: Go here for more info. |
625b17c
to
37d6406
Compare
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🎉
Can you change the commit message to be more descriptive and follow our guidelines more closely; e.g.:
fix(upgrade): do not break if
onMicrotaskEmpty
emits while a$digest
is in progress/* ...explain what this commit fixes... */
Fixes #24680
37d6406
to
8f299b2
Compare
🎉 How's my amended commit message? |
onMicrotaskEmpty
emits while a $digest
is in progress
…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
8f299b2
to
0dde4b5
Compare
merge-assistance: Requires g3sync presubmit. |
@samjulien @gkalpak merging this PR to patch branch failed due to some conflicts, so I merged these changes to master only. Could you please create additional PR with the necessary changes and target patch branch? Thank you. |
With angular#30058, the ngUpgrade internal `angular.module()` method was renamed to `angular.module_()` (to avoid a webpack bug). Merging angular#29794 afterwards resulted in some broken tests, because it still used the old `angular.module()` method name. (The PR had been tested on CI against a revision that did not contain the rename.) This commit fixes the broken tests by renaming the remaining occurrences of `angular.module()`.
On it! Thx, @AndrewKushnir 👍 |
…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
With angular#30058, the ngUpgrade internal `angular.module()` method wa renamed to `angular.module_()` (to avoid a webpack bug). Merging angular#29794 afterwards resulted in some broken tests, because it still used the old `angular.module()` method name. (The PR had been tested on CI against a revision that did not contain the rename.) This commit fixes the broken tests by renaming the remaining occurrences of `angular.module()`.
I have incorporated the change in #30107. |
With #30058, the ngUpgrade internal `angular.module()` method was renamed to `angular.module_()` (to avoid a webpack bug). Merging #29794 afterwards resulted in some broken tests, because it still used the old `angular.module()` method name. (The PR had been tested on CI against a revision that did not contain the rename.) This commit fixes the broken tests by renaming the remaining occurrences of `angular.module()`. PR Close #30126
…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
With #30058, the ngUpgrade internal `angular.module()` method wa renamed to `angular.module_()` (to avoid a webpack bug). Merging #29794 afterwards resulted in some broken tests, because it still used the old `angular.module()` method name. (The PR had been tested on CI against a revision that did not contain the rename.) This commit fixes the broken tests by renaming the remaining occurrences of `angular.module()`. 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
…lar#30126) With angular#30058, the ngUpgrade internal `angular.module()` method was renamed to `angular.module_()` (to avoid a webpack bug). Merging angular#29794 afterwards resulted in some broken tests, because it still used the old `angular.module()` method name. (The PR had been tested on CI against a revision that did not contain the rename.) This commit fixes the broken tests by renaming the remaining occurrences of `angular.module()`. PR Close angular#30126
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. |
PR Checklist
Docs have been added / updated (for bug fixes / features)PR Type
What is the current behavior?
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 theonMicrotaskEmpty
subscription would result in unsubscribing and stop triggering further$digest
s, whenonMicrotaskEmpty
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).Issue Number: #24680
What is the new behavior?
This PR addresses the issue as follows:
$digest
is in progress whenonMicrotaskEmpty
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.Does this PR introduce a breaking change?
Other information
Fixes #24680.