Skip to content
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

Animation logic triggering an unnecessary $rootScope.$digest if ng-if is present #15322

Closed
poshest opened this Issue Oct 27, 2016 · 5 comments

Comments

Projects
None yet
3 participants
@poshest
Copy link
Contributor

commented Oct 27, 2016

In this Plunker, there's a $scope.$apply() because of the ng-click as expected. But then, follows an $evalAsync which calls another, I would argue unnecessary $rootScope.$digest().

It seems to happen because of the presence of the ng-if in

<b ng-if="toggle">toggled</b>

And only happens when toggle evaluates to false, removing the element.

It doesn't happen if the ng-if is changed to ng-show. It seems to be caused by some core Angular animation functionality, involving functions like $$AnimateAsyncRunFactoryProvider, whose waitQueue appears to originate the $evalAsync. I'm not sure what fed the waitQueue in the first place. Although there is animation code involved, this problem occurs whether or not ng-animate is included.

It seems to have appeared in the 1.4.x series. No extra digest happens using v1.3.20.

This is causing many additional digest cycles in my app that I would love to not have.

@poshest poshest changed the title Animation logic triggering an extra (unnecessary?) $rootScope.$digest if ng-if is present Animation logic triggering an unnecessary $rootScope.$digest if ng-if is present Oct 27, 2016

@Narretz

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2016

Even if ngAnimate isn't included, Angular uses the same API for actions that may or may not have animations. And this API is async.
That doesn't mean the additional digest is necessary - this needs some further investigation

@Narretz

This comment has been minimized.

Copy link
Contributor

commented Oct 28, 2016

So ngIf is using a then promise success handler to do some cleanup to the DOM once the element has been removed (this must be done after an animation runs, and the code must be async even if no animation exists). It's possible that we could use the done function for this, which works like then but doesn't use / create a promise. I'm not sure about the implications, though.

@gkalpak

This comment has been minimized.

Copy link
Member

commented Oct 29, 2016

👍 on using .done instead. Since there is already a digest in progress and the extra digest is only for clearing some internal state (previousElements = null), there shouldn't be any implications (famous last words 😁).

Narretz added a commit to Narretz/angular.js that referenced this issue Nov 1, 2016

perf(*): don't trigger additional digests on enter/leave of structura…
…l directives

ngIf, ngInclude, ngSwitch, and ngView now use the `done` callback functions on animation runners returned
by leave/enter animations to do internal cleanup (and $anchorScroll for ngInclude and ngView).
Previously, they were using promise callbacks (`then`), which caused an unnessecary digest.

Background:

In angular@bf0f550, animation
promises where introduced instead of animation callbacks. These promises were however not tied to
the digest cycle, so you had to manually call `$apply` inside them.

This was changed in angular@c8700f0,
so animation promise callbacks would always trigger a `$digest`. This meant that several built-in
directives would now trigger additional digests on leave (ngIf, ngSwitch) or enter/leave (ngInclude,
ngView).

Note that this applies to all calls to $animate.enter/leave, even if no actual animations are running,
because the animation runner code is in the core ng module.

Fixes angular#15322

Narretz added a commit to Narretz/angular.js that referenced this issue Nov 3, 2016

perf(*): don't trigger digests after enter/leave of structural direct…
…ives

ngIf, ngInclude, ngSwitch, and ngView now use the `done` callback functions on animation runners returned
by leave/enter animations to do internal cleanup (and $anchorScroll for ngInclude and ngView).
Previously, they were using promise callbacks (`then`), which caused an unnessecary digest.

Background:

In angular@bf0f550, animation
promises where introduced instead of animation callbacks. These promises were however not tied to
the digest cycle, so you had to manually call `$apply` inside them.

This was changed in angular@c8700f0,
so animation promise callbacks would always trigger a `$digest`. This meant that several built-in
directives would now trigger additional digests on leave (ngIf, ngSwitch) or enter/leave (ngInclude,
ngView).

Note that this applies to all calls to $animate.enter/leave, even if no actual animations are running,
because the animation runner code is in the core ng module.

Fixes angular#15322
@poshest

This comment has been minimized.

Copy link
Contributor Author

commented Nov 4, 2016

Wonderful stuff! Thank you @Narretz, @gkalpak :)

Narretz added a commit to Narretz/angular.js that referenced this issue Nov 22, 2016

perf(*): don't trigger digests after enter/leave of structural direct…
…ives

ngIf, ngInclude, ngSwitch, and ngView now use the `done` callback functions on animation runners returned
by leave/enter animations to do internal cleanup (and $anchorScroll for ngInclude and ngView).
Previously, they were using promise callbacks (`then`), which caused an unnessecary digest.

Background:

In angular@bf0f550, animation
promises where introduced instead of animation callbacks. These promises were however not tied to
the digest cycle, so you had to manually call `$apply` inside them.

This was changed in angular@c8700f0,
so animation promise callbacks would always trigger a `$digest`. This meant that several built-in
directives would now trigger additional digests on leave (ngIf, ngSwitch) or enter/leave (ngInclude,
ngView). The `done` callback, which receives a single argument indicating success / failure was
introduced to allow digest-less responses, but wasn't applied to these directives.

Note that this applies to all calls to $animate.enter/leave, even if ngAnimate isn't included, and
no actual animations are running, because the animation runner code is in the core ng module.

Fixes angular#15322
Closes angular#15345

@Narretz Narretz closed this in f4fb6e0 Nov 23, 2016

@poshest

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2016

Thank you @Narretz :)

Narretz added a commit that referenced this issue Nov 23, 2016

perf(*): don't trigger digests after enter/leave of structural direct…
…ives

ngIf, ngInclude, ngSwitch, and ngView now use the `done` callback functions on animation runners returned
by leave/enter animations to do internal cleanup (and $anchorScroll for ngInclude and ngView).
Previously, they were using promise callbacks (`then`), which caused an unnessecary digest.

Background:

In bf0f550, animation
promises where introduced instead of animation callbacks. These promises were however not tied to
the digest cycle, so you had to manually call `$apply` inside them.

This was changed in c8700f0,
so animation promise callbacks would always trigger a `$digest`. This meant that several built-in
directives would now trigger additional digests on leave (ngIf, ngSwitch) or enter/leave (ngInclude,
ngView). The `done` callback, which receives a single argument indicating success / failure was
introduced to allow digest-less responses, but wasn't applied to these directives.

Note that this applies to all calls to $animate.enter/leave, even if ngAnimate isn't included, and
no actual animations are running, because the animation runner code is in the core ng module.

Fixes #15322
Closes #15345

petebacondarwin added a commit that referenced this issue Nov 23, 2016

perf(*): don't trigger digests after enter/leave of structural direct…
…ives

ngIf, ngInclude, ngSwitch, and ngView now use the `done` callback functions on animation runners returned
by leave/enter animations to do internal cleanup (and $anchorScroll for ngInclude and ngView).
Previously, they were using promise callbacks (`then`), which caused an unnessecary digest.

Background:

In bf0f550, animation
promises where introduced instead of animation callbacks. These promises were however not tied to
the digest cycle, so you had to manually call `$apply` inside them.

This was changed in c8700f0,
so animation promise callbacks would always trigger a `$digest`. This meant that several built-in
directives would now trigger additional digests on leave (ngIf, ngSwitch) or enter/leave (ngInclude,
ngView). The `done` callback, which receives a single argument indicating success / failure was
introduced to allow digest-less responses, but wasn't applied to these directives.

Note that this applies to all calls to $animate.enter/leave, even if ngAnimate isn't included, and
no actual animations are running, because the animation runner code is in the core ng module.

Fixes #15322
Closes #15345

petebacondarwin added a commit that referenced this issue Nov 24, 2016

perf(*): don't trigger digests after enter/leave of structural direct…
…ives

ngIf, ngInclude, ngSwitch, and ngView now use the `done` callback functions on animation runners returned
by leave/enter animations to do internal cleanup (and $anchorScroll for ngInclude and ngView).
Previously, they were using promise callbacks (`then`), which caused an unnessecary digest.

Background:

In bf0f550, animation
promises where introduced instead of animation callbacks. These promises were however not tied to
the digest cycle, so you had to manually call `$apply` inside them.

This was changed in c8700f0,
so animation promise callbacks would always trigger a `$digest`. This meant that several built-in
directives would now trigger additional digests on leave (ngIf, ngSwitch) or enter/leave (ngInclude,
ngView). The `done` callback, which receives a single argument indicating success / failure was
introduced to allow digest-less responses, but wasn't applied to these directives.

Note that this applies to all calls to $animate.enter/leave, even if ngAnimate isn't included, and
no actual animations are running, because the animation runner code is in the core ng module.

Fixes #15322
Closes #15345

ellimist added a commit to ellimist/angular.js that referenced this issue Mar 15, 2017

perf(*): don't trigger digests after enter/leave of structural direct…
…ives

ngIf, ngInclude, ngSwitch, and ngView now use the `done` callback functions on animation runners returned
by leave/enter animations to do internal cleanup (and $anchorScroll for ngInclude and ngView).
Previously, they were using promise callbacks (`then`), which caused an unnessecary digest.

Background:

In angular@bf0f550, animation
promises where introduced instead of animation callbacks. These promises were however not tied to
the digest cycle, so you had to manually call `$apply` inside them.

This was changed in angular@c8700f0,
so animation promise callbacks would always trigger a `$digest`. This meant that several built-in
directives would now trigger additional digests on leave (ngIf, ngSwitch) or enter/leave (ngInclude,
ngView). The `done` callback, which receives a single argument indicating success / failure was
introduced to allow digest-less responses, but wasn't applied to these directives.

Note that this applies to all calls to $animate.enter/leave, even if ngAnimate isn't included, and
no actual animations are running, because the animation runner code is in the core ng module.

Fixes angular#15322
Closes angular#15345
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.