| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
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 by someone other than the pull request submitter. We need to confirm that they're okay 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 cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate. |
Sorry, something went wrong.
|
I have added some additional comments to the docs to indicate some of the dangers of using this feature. |
Sorry, something went wrong.
| 'i18n/**/*.js', | ||
| '!docs/app/assets/js/angular-bootstrap/**', | ||
| '!docs/config/templates/**', | ||
| '!docs/bower_components/**', |
There was a problem hiding this comment.
Is that still needed?
Sorry, something went wrong.
There was a problem hiding this comment.
hmm, well it was causing a problem locally but then perhaps I had some old unused files ...
Sorry, something went wrong.
There was a problem hiding this comment.
removing
Sorry, something went wrong.
| expect(watchSpy).toHaveBeenCalled(); | ||
| })); | ||
|
|
||
| it('should suspend watchers on child scope', inject(function($rootScope) { |
There was a problem hiding this comment.
Maybe test that $resume works in this case as well?
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry, something went wrong.
| * as a child of the directive's containing scope. If the containing scope is suspended the | ||
| * transcluded scope will also be suspended, even if the scope from which the transcluded | ||
| * scope inherits is not suspended. | ||
| * * Multiple directives trying to manage the suspended status of a scope can confuse each other: |
There was a problem hiding this comment.
Yes good point. Perhaps a .$isSuspended() getter or similar so a directive can find out whether its scope is already suspended?
Sorry, something went wrong.
There was a problem hiding this comment.
Hmm this brings up an additional more complex point.
If a parent scope is suspended, but the current scope is not, this scope will not take part in any digests even though it is not officially suspended.
I think that we don't care, since directives that want to suspend a scope don't really care whether an ancestor scope has been suspended. They are only interested in managing their own scope.
The only time this is a problem is if there are multiple directives trying to manage the suspended status of a single scope.
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, I see the problem. It's a fair assumption that if you asked for $isSuspended() you'd want true returned if it was disabled because of a parent, but I think the stuff you put in the docs explaining this issue solves it.
Sorry, something went wrong.
There was a problem hiding this comment.
Out of interest, how hard would it be to implement a .$ancestorIsSuspended()?
Unfortunately my only case for it is if people use $scope.$digest at some point below the $suspend'ed element and want to avoid that $digest in those cases. Since you discourage $scope.$digest, I am girding myself for rejection here, but it would be useful to me, and .$ancestorIsSuspended() would save me writing a custom "recurse up the $parents" function.
Sorry, something went wrong.
There was a problem hiding this comment.
To find out if an ancestor is suspended, you only need to read the $parent scope until you find one that is suspended.
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
Awesome! Thanks guys, and for the code example @gkalpak :)
Sorry, something went wrong.
| * Only use this approach if you are confident that you know what you are doing and have | ||
| * ample tests to ensure that bindings get updated as you expect. | ||
| * | ||
| * Some of the things to consider are: |
There was a problem hiding this comment.
A few other things:
isolate scopes are also suspended (as they are still children of the parent and not the rootScope)
resolved promises e.g. from explicit $q deferreds, timeouts, intervals, and http calls are not bound to scopes and will trigger a global digest if you e.g. execute http calls from a component which has its scope suspended.
Sorry, something went wrong.
|
Added another test, the isSuspended() method, and some more notes to warn developers. |
Sorry, something went wrong.
| * * Transcluded content exists on a scope that inherits from outside a directive but exists | ||
| * as a child of the directive's containing scope. If the containing scope is suspended the | ||
| * transcluded scope will also be suspended, even if the scope from which the transcluded | ||
| * scope inherits is not suspended. |
There was a problem hiding this comment.
I don't know how you managed to explain that in words. Great job! Perhaps an example would make it even more clear? "Eg component B is referenced inside component A, and part of component A's content is transcluded inside component B. If component B's scope is suspended, that part of component A which is transcluded within B will also be scope-suspended." ...if I understood it correctly. Please edit or discard as you see fit!
Sorry, something went wrong.
There was a problem hiding this comment.
I am wary of making this too easy to follow :-P otherwise people who do not understand what they are doing might think they can use this feature :-)
Sorry, something went wrong.
There was a problem hiding this comment.
Fair call! :)
Sorry, something went wrong.
|
I'm very heartened that none of the warnings raised so far and/or added to the docs are surprising or problematic to me. I see this being used in my project and by most people simply to "turn off" whole sub-trees, eg in a version of ng-show that simply stops the $digestion of hidden content, and I don't think any of the mentioned edge cases will cause problems in such an implementation. But the proof is in the pudding. I'll test it soon in my app and give feedback on how it goes in practice. |
Sorry, something went wrong.
There was a problem hiding this comment.
I would consider using terminology that is closer to Angular (e.g. $detach/$attach).
Sorry, something went wrong.
| * @description | ||
| * Resume watchers of this scope subtree in case it was suspended. | ||
| * | ||
| * It is recommended to digest on this scope after it is resumed to catch any modifications |
There was a problem hiding this comment.
It sounds like we recommend $scope.$digest() (which is something we actually recommend against) 😱
Also, it is possible that $resume() will be called during a $digest, in which case you should be triggering a $digest. (Yes, we need a $safeDigest() 😛)
☝️ These should be somehow explained in this doc. Good luck 😁
Sorry, something went wrong.
There was a problem hiding this comment.
Great thanks!
Sorry, something went wrong.
There was a problem hiding this comment.
How about we make $resume trigger an async apply?
Sorry, something went wrong.
There was a problem hiding this comment.
Works for me.
Sorry, something went wrong.
There was a problem hiding this comment.
Does this mean if I have 10 components all resuming at once, there'll be 10x .$applys?
Sorry, something went wrong.
There was a problem hiding this comment.
No, you will get one (as long as they all resume as part of the same macrotask.
Sorry, something went wrong.
| * @kind function | ||
| * | ||
| * @description | ||
| * Call this method to determine if this scope or one of its ancestors have been suspended. |
There was a problem hiding this comment.
or one of its ancestors
Not true (any more?).
Sorry, something went wrong.
There was a problem hiding this comment.
you're right
Sorry, something went wrong.
| * Be aware that a scope may not be included in digests if it has a suspended ancestor, | ||
| * even if `$isSuspended()` returns false. | ||
| * | ||
| * To determine if this scope will be excluded in digests then you must check all its ancestors: |
There was a problem hiding this comment.
I think it might be worth adding a method for that (or support passing a flag to determine whether to check ancestors or not).
Sorry, something went wrong.
There was a problem hiding this comment.
I don't think it is a strong enough use case right now to add it to the core library.
Sorry, something went wrong.
| */ | ||
| $resume: function() { | ||
| this.$$suspended = false; | ||
| }, |
There was a problem hiding this comment.
IMO, $resume should also set dirty = true and lastDirtyWatch = null (in case already during a $digest).
Sorry, something went wrong.
There was a problem hiding this comment.
If $resume triggered an async apply then we wouldn't need to worry about this, no?
Sorry, something went wrong.
There was a problem hiding this comment.
Correct (I think 😁).
Sorry, something went wrong.
There was a problem hiding this comment.
It appears that calling resume from a watch handler is fine, because the fact that the watch handler was called means that the digest is already marked as dirty. So a new digest has already been triggered.
Sorry, something went wrong.
There was a problem hiding this comment.
And we don't seem to need to worry about the short circuiting of lastDirtyWatch:
Sorry, something went wrong.
There was a problem hiding this comment.
So we don't need to run any applyAsync etc.
Sorry, something went wrong.
There was a problem hiding this comment.
Interesting! 👍
Sorry, something went wrong.
| }); | ||
|
|
||
|
|
||
| describe('$suspend/$resume', function() { |
There was a problem hiding this comment.
/$isSuspended 😁
Sorry, something went wrong.
| $rootScope.$resume(); | ||
| expect(childScope.$isSuspended()).toBe(false); | ||
| })); | ||
| }); |
There was a problem hiding this comment.
Ideally, there should be some tests about suspending/resuming during a $digest.
(E.g. to cover basic cases, but also things like #16308 (comment).)
Sorry, something went wrong.
There was a problem hiding this comment.
I'll see what I can add.
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry, something went wrong.
|
Thanks for the review @gkalpak - I am wary of using names that are too similar to Angular, since this is different enough that it could end up being more confusing when migrating to Angular. |
Sorry, something went wrong.
| // yes, this code is a bit crazy, but it works and we have tests to prove it! | ||
| // this piece should be kept in sync with the traversal in $broadcast | ||
| if (!(next = ((current.$$watchersCount && current.$$childHead) || | ||
| if (!(next = ((!current.$$suspended && current.$$watchersCount && current.$$childHead) || |
There was a problem hiding this comment.
The comment above this line says " this piece should be kept in sync with the traversal in $broadcast". So should it? Sorry I'm not familiar enough with the code to know, but I thought it was worth raising just in case.
Sorry, something went wrong.
There was a problem hiding this comment.
Good spot! I think it makes sense to deviate in this case (i.e. suspending the scope should not prevent event propagation), but it is a good idea to reflect that in the comment to avoid future confusion).
Sorry, something went wrong.
There was a problem hiding this comment.
So what does it mean "suspending the scope should not prevent event propagation"? So if a parent scope (not suspended) does a broadcast, any suspended descendants will receive that event and run the associated event code?
If that's what you mean, I think I disagree. I think it's perhaps a more consistent DX if such event receivers are suspended because it's a more absolute and complete "turning off" of the suspended descendants' functionality. Partial suspension is weirder and harder to reason about, document, explain to other developers, IMO.
Code in descendants that react to changes will be arbitrarily contained in event receivers and $watches/$onChanges, and without complete suspension I have to review all of it to make sure I understand which of it will be executed despite suspension and which won't.
Sorry, something went wrong.
There was a problem hiding this comment.
I think I agree with @poshest here. We should not pass broadcasts to suspended scopes.
Sorry, something went wrong.
There was a problem hiding this comment.
I think I disagree with both of you then 😁
In my view $suspend/$resume is only related to change-detection (which in AngularJS is performed via $digest runs). Event propagation is an orthogonal concern and should be able to work independently from CD.
Suspending event propagation sounds to me a little like saying we will not trigger $timeouts if they come from a suspended scope (assuming we could determine the scope). Or not evaluate $eval()/$evalAsync() expressions.
IMO it would be way more confusing to stop propagating events.
Secondary reasons:
Sorry, something went wrong.
There was a problem hiding this comment.
Sorry, something went wrong.
There was a problem hiding this comment.
Well, I still feel that it's less confusing to have the descendants suspended in every respect, so yes, it would be a clearer API to include suspension of $timeouts, and $eval()/$evalAsync()s. But you make it sound like implementation of that would be difficult. :P
By "By running a $digest once the scope is resumed", are you saying that the missed events would have, if not for being $suspended, affected a component's state and therefore the component will show the wrong state when it's $resumed? I wouldn't code a component like that. But I guess there may be use cases I haven't thought of.
"Events could be a handy way to trigger $suspend/$resume". True! Certainly if the original intent to suspend the current scope (not only descendent ones) comes to fruition, this would be crucial. But even then, I still prefer suspending only descendants.
While it's not my preference, I'm open to excluding $broadcasts from suspending scopes, and in that case I'd argue for
Sorry, something went wrong.
There was a problem hiding this comment.
Well, I still feel that it's less confusing to have the descendants suspended in every respect, so yes, it would be a clearer API to include suspension of $timeouts, and $eval()/$evalAsync()s. But you make it sound like implementation of that would be difficult.
It's difficult / impossible, because ...
For all of the following, the scope and its descedants are excluded from any digests: $apply, $evalAsync, $timeout, $interval, $q, $http. This is because all of these trigger a digest on the $rootScope, not the current scope, so they will digest until they reach the suspended scope.
So these functions are not bound to a specific scope, but to the change detection (digest cycle) in general.
timeout and interval have an option to skip the apply for that reason. Http could get one, but it's trickier: #12557. eval() doesn't trigger a digest.
Essentially when devs use these functions they need to know that they suspend the digest, but not the triggering of the change detection.
Sorry, something went wrong.
|
OK, I think I have found a possible Catch 22 with this implementation from my early testing. I started by creating my most desired perf feature since one-time binding. At the moment it's just a hack of ng-show. I call it p-show ("p" for "performance"! :D). The idea is to disable the descendants' scopes upon hide, and re-enable upon show. angular.module('myApp').directive('pShow', ['$animate', function($animate) {
return {
restrict: 'A',
multiElement: true,
link: function(scope, element, attr) {
scope.$watch(attr.pShow, function pShowWatchAction(value) {
// ADDED poshest
var NG_HIDE_CLASS = 'ng-hide'; // to remain compatible with original ng-show code
var NG_HIDE_IN_PROGRESS_CLASS = 'ng-hide-animate'; // ditto
if (value) scope.$resume();
else scope.$suspend();
console.log('pShow:', value, '$isSuspended:', scope.$isSuspended());
// END: ADDED poshest
// we're adding a temporary, animation-specific class for ng-hide since this way
// we can control when the element is actually displayed on screen without having
// to have a global/greedy CSS selector that breaks when other animations are run.
// Read: https://github.com/angular/angular.js/issues/9103#issuecomment-58335845
$animate[value ? 'removeClass' : 'addClass'](element, NG_HIDE_CLASS, {
tempClasses: NG_HIDE_IN_PROGRESS_CLASS
});
});
}
};
}]);
I realised the problem pretty early on... in almost all the use cases for .$suspend() that I can think of, the .$resume() code will need to be located INSIDE a $watch or $onChanges on that very suspended scope! The result is that hiding works, but showing doesn't. I read back to vahan-hartooni's version of this and he, I think rightly wrote this as a suspension of child scopes only. I don't see how also suspending the current scope could be workable, unless anyone has other ideas about how I can trigger .$resume() in an "Angular"-way or if I've mis-used or mis-understood scope.$suspend. |
Sorry, something went wrong.
|
I think you want to make the directive create a new scope, e.g. scope: true in the declaration... |
Sorry, something went wrong.
|
I added scope: true to the declaration. That caused this error in multiple places Multiple directives [pShow, myComponent] asking for new/isolated scope..., so I changed all of these <my-component p-show="$ctrl.showMe"></my-component> to these <div p-show="p-show="$ctrl.showMe"><my-component></my-component></div> and I still get the same result. I've got a deadline til Sunday night on something else, but I'll get a codepen going on Monday so we're on the same page and can eliminate the possibility that I messed something silly up. Enjoy your weekends! :) |
Sorry, something went wrong.
|
Ahah! Of course, the component is requesting an isolate scope, so you can't also ask for a new scope :-( |
Sorry, something went wrong.
|
Another quick observation while I think of it is that $scope.$digests which are manually called on descendant scopes are still firing despite an ancestor having been suspended. I expect this based on my understanding of the implementation (.$$suspended isn't propagated down the tree), but it might be confusing to others in cases when they do stuff outside Angular and then call a $digest on the descendant to get Angular back in sync. They may expect that $digest not to fire because of the suspended ancestor. Maybe just another note for the docs, perhaps as a clarification to the note...
Those digests need to be initiated from the parent or above, right? |
Sorry, something went wrong.
|
Thanks @poshest for doing the tests! I think you are right with your concerns. So in general, I think it makes sense to suspend the child scopes when calling suspend on a scope. I assume a general use case will be to cut of whole parts of an application from digesting if they are hidden, e.g. if a modal is open, you don't need to digest the rest of the UI. That means most of the time the control of the suspension does not lie with the scope owner ( e.g. component), but with the parent. For example, if I want to use a third party component, I should be able to put a "suspender" directive on the element, that will toggle digestion on the component's scope (template) (the part which I don't own), while still allowing the component element (which might have other bindings) to digest. At the moment, this seems like it's unsupported. Manual $digests will digest the scope on which they are called, and all descendant scopes. This is to be expected. You should use scope.$apply anyway, which does not have this problem - it digest the rootScope and will stop on the suspended scope. |
Sorry, something went wrong.
|
Is there actually anything wrong with @Narretz's example directive? Does it work if there is an isolate component on the element being transcluded? |
Sorry, something went wrong.
Why would this need to be shipped with core? |
Sorry, something went wrong.
|
Because of
i.e. you need one scope that handles the watch that suspends / unsuspends another scope. You can't have both on the same scope, because the watch won't fire on a suspended scope. |
Sorry, something went wrong.
|
Here is a directive that appears to work as I would expect: http://plnkr.co/edit/N0w01zkzUPugUJ10wRUp?p=preview Can you comment? |
Sorry, something went wrong.
|
The transclusion trick works for me and so do your directives @Narretz and @petebacondarwin. They resolve my issue of $watch disabling, albeit in a way that I wouldn't easily come to myself. The suspend children/descendants method might be more intuitive for less experienced developers, but then as you said, this feature isn't for them :) |
Sorry, something went wrong.
|
Since it is a bit tricky to get it to work just-so, how about we do implement an ngSuspend directive, that is similar to what @Narretz and I have been suggesting? But lace its documentation with a lot of warnings... |
Sorry, something went wrong.
| * * If two directives suspend a scope, then one of them resumes the scope, the scope will no | ||
| * longer be suspended. This could result in the other directive believing a scope to be | ||
| * suspended when it is not. | ||
| * * If a parent scope is suspended then all its descendants will be excluded from future digests |
There was a problem hiding this comment.
...will be excluded from future digests that are initiated from the parent or any ancestor of the parent (which includes every .$apply()) whether or not...
Sorry, something went wrong.
|
@petebacondarwin I'm happy either way. :) But I'm curious why not just go with the suspend children option? I don't really understand @Narretz
Why does that matter? If there are no child scopes there's nothing to suspend (works as expected). The suspension will still work if that scope later acquires children, no? |
Sorry, something went wrong.
|
I guess. I haven't looked into the implementation. I think it's easy to break the scope traversion loop when we find a scope that whose children are suspended. @petebacondarwin directive has the advantage that you can put it on the same element. |
Sorry, something went wrong.
vahan-hartooni's version of this simply omits the watchers = !current.$$suspended && bit from if ((watchers = !current.$$suspended && current.$$watchers)) {
So that effectively only the change to this line if (!(next = ((!current.$$suspended && current.$$watchersCount && current.$$childHead) remains. Does that effectively target only the child/descendent scopes? Or is this a naive approach? |
Sorry, something went wrong.
|
As a thought experiment consider the following situation where there is a directive suspendOn that doesn't create a scope of its own but toggles the suspended state of the current scope. <div ng-controller="MyController"> <!-- this creates a scope for us -->
<div>{{ a }}</div>
<div suspend-on="value">
{{ b }}
<some-component></some-component>
</div>
<div>{{ c }}</div>
</div>The scope at the suspendOn directive is the one defined by the controller. a) If we go with the current implementation, then such a directive would suspend updates to all the interpolations ({{ a }}, {{ b }} and {{ c }}) and the someComponent component. In both cases this is crazy and confusing. So, our suspend-on directive must create a scope of its own in order to work in an intuitive sense. As @poshest pointed out, you cannot simply insist on suspendOn asking for a new scope since that prevents it from being put on an element that is also a component. This leaves us requiring that suspendOn uses the transclusion approach to ensure that a new scope is created. Further by using transclude: 'element' we get the bonus that we can put it on an element that has a component and that component will either appear inside or outside the suspended scope depending upon its priority. If the component has a lower priority then it will be part of the transclusion, if it is higher then the suspendOn directive will be part of the component's transclusion. For me, this makes sense. I believe that changing to suspend children rather than suspending the current scope is no more intuitive than what this PR proposes, since the directive that would need to be implemented is just as complex. |
Sorry, something went wrong.
|
I agree that suspending only children is tricky/not ideal (but probably for different reasons than @Narretz 😁). Basically, change detection should be a responsibility of the component that contans the bindings, not the user of the component. The primary (only) reason for suspending a scope is performance and only the implementor should be concerned with when is an appropriate time to suspend/resume. The user of the component should not know anything about that and not care. For example, imagine a component that displays user info and only cares to update its view when the logged-in user changes. It suspends its scope once the use logs in and temporarily resume once the UserService emits a user change. This is an internal implementation detail that should not leak outside of the component. The consumer should just drop the component on a page/template and not care what the component does to keep itself efficient and when it needs to be resumed. By suspending children only we make it essentially impossible for a component to control its own change detection (unless every components that needs that adds an extra wrapper in its template and creates a new scope for it). Furthermore, this approach deviates from how things work in Angular (2+). Although we can't get a 1-to-1 match in bahavior, keeping the concepts and usage patterns as similar as possible would make sense to me. Still, I don't have a brilliant alternative suggestion 😁 Maybe the $suspend()/$resume() primitives are just too low-level to bake into the component pattern and we can instead build on top of them. Ideally, I would like an easy way to comfigure this on the CDO (Component Definition Object) - similar to Angular's changeDetection: ChangeDetectionStrategy.OnPush - and an easy way to request a one-off change detection (or automatically set it up when $onChanges() fires). But I think these can be built on top of $suspend()/$resume() as non-breaking features. In any case, I wouldn't go too far recommending an alternative usage pattern (e.g. as shown with the transclude directives). |
Sorry, something went wrong.
|
Woohoo! I monkey-patched my way to (a naive implementation of) ChangeDetectionStrategy.OnPush 🎉 |
Sorry, something went wrong.
|
Looks like you and I look at this from 2 different perspectives @gkalpak :>
|
Sorry, something went wrong.
|
Darn, you're absolutely right. $suspend either way without some kind of scope transclusion makes no sense. I'm really warming to an ngSuspend. How would that work if you had <div ng-if="myVar" ng-suspend="myVar"></div>? Do they both create their own scope? Does it matter if you have multiple transcluded scopes cascading from the same element? I guess it's no different from something like <div ng-if="myVar" ng-repeat="..."></div> and the priority determines the order of transclusion? Oh, and that just made me think... in the case of <div ng-suspend="myVar" ng-repeat="..."></div>, I would expect ng-suspend to suspend all of the ng-repeat scopes. Is that how it would work (given the appropriate priority)? My problem with your "user info" example is that your use case is a simple component with no children appearing once on the page. This is not a situation calling for perf optimisation. AngularJS suffers in situations when you have massive ng-repeat lists. So a better example is a "report" component, which has many table rows. Even if the parent report isn't scope-suspended, all of its ng-repeated child scopes will be, which gives you the orders of magnitude perf improvement you're looking for. In any case, I believe Pete's thought experiment renders this thread redundant. On the other hand, ChangeDetectionStrategy.OnPush? AMAZING DUDE! That is way beyond expectations, but would be an equally, if not more important perf feature for me. Would it be possible to add a markForCheck() equivalent? That would work best with my architecture where I'm using something like observables, but if I have to change everything to immutables to take advantage, I won't be unhappy. Maybe...? Definitely! :) Oh, it's gonna be a great Christmas :) |
Sorry, something went wrong.
|
@poshest regarding usage of ng-suspend with ng-if and ng-repeat it comes down to priority. All three directives use transclude: 'element' which passes lower priority directives down to the transcluded content. |
Sorry, something went wrong.
|
@poshest, @gkalpak, @Narretz and @mgol are we agreed that merging this PR with just the suspend functionality is an acceptable start? We can discuss building on top of it later? Would it be a breaking change to start by not suspending $broadcast events but then adding it in later? |
Sorry, something went wrong.
|
@petebacondarwin sure, let's merge now and make some progress with this. With all that I've learned here, $suspend as implemented in this PR is plenty enough for me to use for everything I'd hoped. Do you mean will the later adding of $broadcast suspension be a breaking change? Yes, I think so. I prefer to resolve that now and not create expectations of future changes. I'm happy to go with not suspending $broadcasts as the "final" solution. I still argue that some developers might find this confusing, but with appropriate warnings in the documentation it can work. On the other hand, not suspending $broadcasts gives the developer more options, as @gkalpak argues
And we can always manually "ignore" broadcasts on suspended tree branches by checking for ancestries' .isSuspended(). |
Sorry, something went wrong.
|
I think suspending the events would be a breaking change. I also don't think we should suspend / stop the events anyway because they are independent from the digest cycle. |
Sorry, something went wrong.
| * Some of the things to consider are: | ||
| * | ||
| * * Any external event on a directive/component will not trigger a digest while the hosting | ||
| * scope is suspended - even if the event handler calls `$apply()` or `$digest()`. |
There was a problem hiding this comment.
Well...if the scope itself isn't suspended, scope.$digest() will run change detection, no?
Maybe change to "scope.$apply() or $rootScope.$digest()" to be more explicit.
Sorry, something went wrong.
| * transcluded scope will also be suspended, even if the scope from which the transcluded | ||
| * scope inherits is not suspended. | ||
| * * Multiple directives trying to manage the suspended status of a scope can confuse each other: | ||
| * * A call to `$suspend` an already suspended scope is a no-op. |
There was a problem hiding this comment.
an already --> on an already (?)
Sorry, something went wrong.
| * scope inherits is not suspended. | ||
| * * Multiple directives trying to manage the suspended status of a scope can confuse each other: | ||
| * * A call to `$suspend` an already suspended scope is a no-op. | ||
| * * A call to `$resume` a non-suspended scope is a no-op. |
There was a problem hiding this comment.
a non-suspended --> on a non-suspended (?)
Sorry, something went wrong.
| * included in future digests until all its ancestors have been resumed. | ||
| * * Resolved promises, e.g. from explicit `$q` deferreds and `$http` calls, trigger `$apply()` | ||
| * against the `$rootScope` and so will still trigger a global digest even if the promise was | ||
| * initialiated by a component that lives on a suspended scope. |
There was a problem hiding this comment.
initialized + created = initialiated 😃
Sorry, something went wrong.
There was a problem hiding this comment.
initialized + initiated
Sorry, something went wrong.
| */ | ||
| $resume: function() { | ||
| this.$$suspended = false; | ||
| }, |
There was a problem hiding this comment.
Interesting! 👍
Sorry, something went wrong.
|
Sorry for the late replies 😞 Here you go:
The point is that they don't know if a component needs to be updated or not and they shouldn't know imo. (There are implementation details that only the component author should know and care about.)
Oh, good point (sadly) 😞 Fair enough then 😄
It was just an example. The exact same applies to a super-heavy table component with hundreds of ngRepeated rows and cells. The component itself should be in charge of whether/when the scopes can be suspended' i.e. the component author, not the component consumer.
Did it? 😛
In my PoC, I add the checkOnce() method on the Scope prototype, which I think is the equivalent of markForCheck(). I.e. it temporarily resumes parent scopes and re-suspends them after one digest. I am still pretty happy with how easy-ish it was to implement an OnPuch PoC on top of this PR 🎉 |
Sorry, something went wrong.
|
TODO:
|
Sorry, something went wrong.
This can be very helpful for external modules that help making the digest loop faster by ignoring some of the watchers under some circumstance. Example: https://github.com/shahata/angular-viewport-watch Thanks to @shahata for the original implementation. Closes angular#5301
|
Thank you Pete and everyone for all your work on this. It will make a big difference to me and I hope others will find it useful. |
Sorry, something went wrong.
This can be very helpful for external modules that help making the digest loop faster by ignoring some of the watchers under some circumstance. Example: https://github.com/shahata/angular-viewport-watch Thanks to @shahata for the original implementation. Closes #5301 Closes #16308
| Back | FazBrowse Home | New Git URL |
This is a rebase of @shahata's original PR at #10658.
We need to think about @lgalfaso's concerns (#10658 (comment) and #10658 (comment)) before merging...