Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

feat(rootScope): allow $apply expressions to prevent unnecessary digest #8301

Closed
wants to merge 2 commits into from
Closed

feat(rootScope): allow $apply expressions to prevent unnecessary digest #8301

wants to merge 2 commits into from

Conversation

shahata
Copy link
Contributor

@shahata shahata commented Jul 22, 2014

This allows, for example to prevent unnecessary digest in ngClick handler function in case the handler does not have any immediate side effects.

Closes #8286

@mary-poppins
Copy link

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

  • Uses the issue template (#8301)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

@shahata
Copy link
Contributor Author

shahata commented Jul 22, 2014

This means that scope.$apply('$abortApply()') and ng-click="$abortApply()", for example, will prevent the digest. In case this is something desirable, I can add the necessary docs.

@shahata
Copy link
Contributor Author

shahata commented Nov 29, 2014

Any thoughts about this one? I really like it and I even thought about adding another option $localDigest so that those directive will be able to do scope.$digest instead of scope.$apply if the developer doesn't want to run a full digest on $rootScope. I just added a duplicate of ngMouseover to my project because a digest was needed only a small fraction of the times that handler was invoked and I think it makes sense to include this in angular core.

@googlebot
Copy link

CLAs look good, thanks!

…gest

This allows, for example to prevent unnecessary digest in ngClick handler function in case the handler does not have any immediate side effects.

Closes #8286
This allows to prevent unnecessary watchers to be invoked in case the handler side effects are limited to a certain scope

Closes #7298
@shahata
Copy link
Contributor Author

shahata commented Dec 14, 2014

Added support also for ng-click="$partialDigest()" and ng-click="$partialDigest($parent)" to allow the click handler to limit the digest to the current scope (or some specific scope) instead of $rootScope

Closes #7298

@lgalfaso
Copy link
Contributor

The main issue with this PR is that $rootScope already provides all the APIs to do partial digest and no digest at all. We should not be adding 10s of lines of code to $rootScope, and duplicate functionality already present there so a directive does not need to add 1-2 lines of code

@shahata
Copy link
Contributor Author

shahata commented Jan 10, 2015

I agree that the purpose of this change is only so that people will not have to duplicate directives, but I think it is a common enough use case that we can provide them the tools they need and not force them to duplicate code.

@lgalfaso
Copy link
Contributor

Adding duplicate code/multiple ways to do the same thing in a critical component as it is $rootScope, is not nice.
BTW, there are too many edge cases that we should not care about. Eg, what should we do in each of these?

ng-click="a = 1; $partialDigest(); $partialDigest($parent.$nextSibling)"
ng-click="a = 1; $partialDigest(); $AbortDigest()"

@lgalfaso
Copy link
Contributor

Merging this would make the migration to Angular 2 harder. I am going to close this.

In case I am missing something, feel free to reopen this issue

@lgalfaso lgalfaso closed this Mar 10, 2015
@dragosrususv
Copy link

I would have envisioned this with a lot less simpler interface. Just adding some options similar to ng-model-options to the directives themselves:
Example

The above gives more power over the mechanics under the hood, but that is fine too as long as it fixes the problem.

@lgalfaso : Excuse me to say, but I find it very hard to understand your mode of thinking. These are problems that the community are facing TODAY and you are closing a PR on a project which will probably not be maintained in 2 years and you are pointing out to a FUTURE AngularJS 2.0 project in which NOBODY could implement anything as of today. How is this helping? What am I missing here?

@realityking
Copy link
Contributor

@dragosrususv that wouldn't work with multiple event handler when only some should skip the digest

@dragosrususv
Copy link

@realityking - touche. ng-mouse-enter-options? :)
I wanted to make it easier for @shahata because he's a nice guy :)

UPDATE: or maybe ng-options="{mouseEnter: {runApply: false}}" --- but that's like entering in a json programmers world.

@lgalfaso
Copy link
Contributor

@dragosrususv I understand that this might be frustrating, but not everything belongs to the core. In this specific case, if you think that there is a big need for directives that would handle events without triggering $apply, then create a module with such directives and put it in github. There are no core changes needed just normal directives.

@dragosrususv
Copy link

@lgalfaso : normal directives trigger a $digest/refresh on the whole application instead of just the current $scoping area. PERF wise, that's a disaster. And AngularJS has the right tools to fix this, even if backward compatibility is kept.

Still, I do agree that people who actually have this problem (or find the root cause of it!) would fall into the 2% category and it should only be added in framework as an option at most. I'll think about adding a module with such light directives, but I would prefer for people to have this by default instead of adding another bower dep.

@lgalfaso
Copy link
Contributor

@dragosrususv I understand this may be frustrating, but there are two camps here:

  • Developers who think that is a feature makes sense for a given % of developers, then it should be in the core
  • Developers that think that Angular core should be small as they are already struggling with the download size of their application, and adding more features that they do not use adds bytes to the app download

Given that it is a lot easier to just add another library than to selectively remove chunks to save bytes, is that before something is added, we are forced to think several times

  • Is this something that belongs to the core?
  • Can this be done thru a third party module?
  • What would be the impact on the previous two camps

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make ngClick only $apply when it doesn't return a promise
8 participants