feat($rootScope): allow suspending and resuming watchers on scope #16308

petebacondarwin · 2017-10-31T11:48:55Z

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...

googlebot · 2017-10-31T11:48:58Z

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.

petebacondarwin · 2017-10-31T13:32:41Z

I have added some additional comments to the docs to indicate some of the dangers of using this feature.

mgol · 2017-10-31T13:38:59Z

Gruntfile.js

@@ -178,6 +178,7 @@ module.exports = function(grunt) {
          'i18n/**/*.js',
          '!docs/app/assets/js/angular-bootstrap/**',
          '!docs/config/templates/**',
+          '!docs/bower_components/**',


Is that still needed?

hmm, well it was causing a problem locally but then perhaps I had some old unused files ...

mgol · 2017-10-31T13:40:52Z

test/ng/rootScopeSpec.js

+        expect(watchSpy).toHaveBeenCalled();
+      }));
+
+      it('should suspend watchers on child scope', inject(function($rootScope) {


Maybe test that $resume works in this case as well?

See https://github.com/angular/angular.js/pull/16308/files#diff-bab291b2ff3232222e01694707b01a1dR1286

poshest · 2017-11-01T14:03:46Z

src/ng/rootScope.js

+       *   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:


Yes good point. Perhaps a .$isSuspended() getter or similar so a directive can find out whether its scope is already suspended?

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.

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.

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.

To find out if an ancestor is suspended, you only need to read the $parent scope until you find one that is suspended.

An example is shown here.
@poshest, if you want it to be available on every scope, you can put it on $rootScope's prototype.

Awesome! Thanks guys, and for the code example @gkalpak :)

Narretz · 2017-11-01T15:30:34Z

src/ng/rootScope.js

+       * 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:


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.

petebacondarwin · 2017-11-01T17:09:28Z

Added another test, the isSuspended() method, and some more notes to warn developers.

poshest · 2017-11-02T08:47:36Z

src/ng/rootScope.js

+       * * 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.


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!

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 :-)

Fair call! :)

poshest · 2017-11-02T10:02:39Z

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.

gkalpak

I would consider using terminology that is closer to Angular (e.g. $detach/$attach).

gkalpak · 2017-11-03T11:56:03Z

src/ng/rootScope.js

+       * @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


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 😁

Great thanks!

How about we make $resume trigger an async apply?

Works for me.

Does this mean if I have 10 components all resuming at once, there'll be 10x .$applys?

No, you will get one (as long as they all resume as part of the same macrotask.

gkalpak · 2017-11-03T11:57:21Z

src/ng/rootScope.js

+       * @kind function
+       *
+       * @description
+       * Call this method to determine if this scope or one of its ancestors have been suspended.


or one of its ancestors

Not true (any more?).

you're right

gkalpak · 2017-11-03T11:59:15Z

src/ng/rootScope.js

+       * 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:


I think it might be worth adding a method for that (or support passing a flag to determine whether to check ancestors or not).

I don't think it is a strong enough use case right now to add it to the core library.

gkalpak · 2017-11-03T12:04:28Z

src/ng/rootScope.js

+       */
+      $resume: function() {
+        this.$$suspended = false;
+      },


IMO, $resume should also set dirty = true and lastDirtyWatch = null (in case already during a $digest).

If $resume triggered an async apply then we wouldn't need to worry about this, no?

Correct (I think 😁).

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.

And we don't seem to need to worry about the short circuiting of lastDirtyWatch:

if the resumed scope is "after" the last dirty watch, then it will have been digested on that turn of the digest cycle.

if the resumed scope is "before" the last dirty watch, then it will get digested before we hit the short circuit.

So we don't need to run any applyAsync etc.

Interesting! 👍

gkalpak · 2017-11-03T12:04:50Z

test/ng/rootScopeSpec.js

@@ -1255,6 +1255,84 @@ describe('Scope', function() {
      });
    });

+
+    describe('$suspend/$resume', function() {


/$isSuspended 😁

gkalpak · 2017-11-03T12:10:25Z

test/ng/rootScopeSpec.js

+        $rootScope.$resume();
+        expect(childScope.$isSuspended()).toBe(false);
+      }));
+    });


Ideally, there should be some tests about suspending/resuming during a $digest.
(E.g. to cover basic cases, but also things like #16308 (comment).)

I'll see what I can add.

See https://github.com/angular/angular.js/pull/16308/files#diff-bab291b2ff3232222e01694707b01a1dR1317 and https://github.com/angular/angular.js/pull/16308/files#diff-bab291b2ff3232222e01694707b01a1dR1340

petebacondarwin · 2017-11-03T22:10:31Z

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.

poshest · 2017-11-04T12:11:48Z

src/ng/rootScope.js

@@ -855,7 +857,7 @@ function $RootScopeProvider() {
            // Insanity Warning: scope depth-first traversal
            // 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) ||


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.

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).

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.

I think I agree with @poshest here. We should not pass broadcasts to suspended scopes.

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:

By running a $digest once the scope is resumed, you should be able to get to the same state you would have gotten if the scope was not suspended (most of the time). I.e. you are able to "replay" the changes, because they are recorded in the app state. This is especially true if (as recommended) you watch expressions and handlers are idempotent.

Events could be a handy way to trigger $suspend/$resume 😁

I think it makes sense to handle digest and events separately here, as scope events are quite different from the digest cycle, and could be useful as @gkalpak said, to control the suspension. If we want to give devs the option to shut down broadcasting, then we need something like this PR: #15877

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

changing the names to $suspendDigests() and $resumeDigests() and $digestsAreSuspended() to be clearer

adding my proposed $ancestorIsSuspended() ($ancestorDigestsAreSuspended()!) so that developers could easily avoid reacting to $broadcasted events on suspended scopes on a case-by-case basis.

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.

poshest · 2017-11-04T14:27:28Z

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.

petebacondarwin · 2017-11-04T16:11:43Z

I think you want to make the directive create a new scope, e.g. scope: true in the declaration...
Then you can watch the attribute value which will be bound to the parent scope.

poshest · 2017-11-04T19:55:33Z

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! :)

petebacondarwin · 2017-11-04T19:59:03Z

Ahah! Of course, the component is requesting an isolate scope, so you can't also ask for a new scope :-(

poshest · 2017-11-04T20:20:02Z

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...

If a parent scope is suspended then all its descendants will be excluded from future digests...

Those digests need to be initiated from the parent or above, right?

Narretz · 2017-11-06T11:37:59Z

Thanks @poshest for doing the tests! I think you are right with your concerns.
But first, here's a directive that would make "suspend scope on hide" work: http://plnkr.co/edit/5Ci3xUAmEduroFUF31LK?p=preview
It uses transclusion into a child scope (instead of the parent scope) to make it work.
The problem is that we'd have to ship such a directive as part of core, as it would need to be implemented anyway if someone wants to use this feature.

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.

petebacondarwin · 2017-11-13T17:56:42Z

Is there actually anything wrong with @Narretz's example directive? Does it work if there is an isolate component on the element being transcluded?

petebacondarwin · 2017-11-13T17:57:13Z

The problem is that we'd have to ship such a directive as part of core, as it would need to be implemented anyway if someone wants to use this feature.

Why would this need to be shipped with core?

Narretz · 2017-11-13T18:15:55Z

Because of

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.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.

petebacondarwin · 2017-11-13T21:42:18Z

Here is a directive that appears to work as I would expect: http://plnkr.co/edit/N0w01zkzUPugUJ10wRUp?p=preview

Can you comment?

poshest · 2017-11-14T08:12:15Z

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 :)

petebacondarwin · 2017-11-14T10:36:49Z

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...
Alternatively we just create a separate module that contains this directive and release it as a third party library and then link to it from our documentation?

poshest · 2017-11-14T10:49:47Z

src/ng/rootScope.js

+       *    * 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


...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...

poshest · 2017-11-14T10:57:26Z

@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

Technically I think suspending only the children is a bit more difficult because you cannot be sure that the scope you call it on has a child scope at the moment.

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?

Narretz · 2017-11-14T12:24:24Z

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.
However, the directive also suspends the host element, not only the component contents. I find that a bit unflexible. My previous example: if I include a 3rd party component which I want to suspend, but still be able to change bindings on the host element, e.g. http://plnkr.co/edit/xEkRBjswFMrcdEF5SG0x?p=preview
You can see that the color only changes if I digest the component. To me, that's a least unexpected. I'd find it easier to understand if only the child (isolate) scope was suspended.

poshest · 2017-11-14T12:41:48Z

I think it's easy to break the scope traversion loop when we find a scope that whose children are suspended.

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?

petebacondarwin · 2017-11-14T14:16:33Z

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.
b) If we go with suspending children implementation, then the template/transclusion of someComponent will be suspended (since it creates a new isolate/transclusion scopes) but {{ b }} will not be suspended.

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.

gkalpak · 2017-11-14T14:50:08Z

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).
This is also the reason why I don't particularly like the transclude directives posted above.

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).

gkalpak · 2017-11-14T15:05:32Z

Woohoo! I monkey-patched my way to (a naive implementation of) ChangeDetectionStrategy.OnPush 🎉

Demo

Narretz · 2017-11-14T16:47:50Z

Looks like you and I look at this from 2 different perspectives @gkalpak :>
I agree that it makes sense for components to decide on their update strategy.
But if the component owner wants to cut off a part of the page from the digest cycle, why shouldn't he? If he knows that this component won't need to be updated, then it should be possible to do that. Ofc a component could use onPush, but what if you are using a component that is unlikely to get updated with this new API? Then you want to be able to suspend it completely yourself.
So I think both are valid use cases. Yours, because it makes it easier to write Angular2 style apps, and mine because it makes it easier to perf-tune existing apps.
So maybe we

keep it so that the actual scope gets suspended
add the transclude-suspend directive
add an "onPush" option

poshest · 2017-11-15T11:25:28Z

@petebacondarwin

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)?

@gkalpak

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.

@Narretz

Maybe...? Definitely! :) Oh, it's gonna be a great Christmas :)

petebacondarwin · 2017-11-15T17:37:06Z

@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.

petebacondarwin · 2017-11-17T12:58:24Z

@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?

poshest · 2017-11-17T16:15:25Z

@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

keep state up-to-date where events change component state
potentially use events to trigger suspends/resumes

And we can always manually "ignore" broadcasts on suspended tree branches by checking for ancestries' .isSuspended().

Narretz · 2017-11-17T16:44:28Z

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.
Otherwise, 👍 for merging with just the suspend functionality.

gkalpak · 2017-11-22T12:41:22Z

src/ng/rootScope.js

+       * 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()`.


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.

gkalpak · 2017-11-22T12:42:18Z

src/ng/rootScope.js

+       *   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.


an already --> on an already (?)

gkalpak · 2017-11-22T12:42:43Z

src/ng/rootScope.js

+       *   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.


a non-suspended --> on a non-suspended (?)

gkalpak · 2017-11-22T12:44:40Z

src/ng/rootScope.js

+       *   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.


initialized + created = initialiated 😃

initialized + initiated

gkalpak · 2017-11-22T13:04:46Z

src/ng/rootScope.js

+       */
+      $resume: function() {
+        this.$$suspended = false;
+      },


Interesting! 👍

gkalpak · 2017-11-22T13:20:03Z

Sorry for the late replies 😞 Here you go:

@Narretz

But if the component owner wants to cut off a part of the page from the digest cycle, why shouldn't he? If he knows that this component won't need to be updated, then it should be possible to do that.

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.)

but what if you are using a component that is unlikely to get updated with this new API? Then you want to be able to suspend it completely yourself. So I think both are valid use cases.

Oh, good point (sadly) 😞 Fair enough then 😄

@poshest

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.

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.
But there is the point @Narretz made about applying this feature to existing 3rd-party components.

In any case, I believe Pete's thought experiment renders this thread redundant.

Did it? 😛

On the other hand, ChangeDetectionStrategy.OnPush? [...]
Would it be possible to add a markForCheck() equivalent?

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.
But being a PoC, it is just a naive implementation that doesn't account for scopes being otherwise resumed during the digest (i.e. it will always re-suspend them after the digest and this might not be what you want).
In the "real thing", we should properly handle these edge-cases.

I am still pretty happy with how easy-ish it was to implement an OnPuch PoC on top of this PR 🎉

petebacondarwin · 2017-11-27T19:28:34Z

TODO:

- change comment to reflect that digests and broadcasts are no longer in sync

@shahata

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

poshest · 2017-12-11T21:10:46Z

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.

@shahata

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

petebacondarwin added the type: feature label Oct 31, 2017

petebacondarwin added this to the 1.7.0 milestone Oct 31, 2017

googlebot added cla: no labels Oct 31, 2017

petebacondarwin mentioned this pull request Oct 31, 2017

feat($rootScope): allow suspending and resuming watchers on scope #10658

Closed

angular deleted a comment from googlebot Oct 31, 2017

petebacondarwin mentioned this pull request Oct 31, 2017

Support detaching/re-attaching scope #5301

Closed

petebacondarwin force-pushed the suspend-scope branch from 47b7a72 to 4951709 Compare October 31, 2017 13:31

petebacondarwin self-assigned this Oct 31, 2017

mgol suggested changes Oct 31, 2017

View reviewed changes

poshest reviewed Nov 1, 2017

View reviewed changes

Narretz reviewed Nov 1, 2017

View reviewed changes

petebacondarwin force-pushed the suspend-scope branch from 4951709 to 641f8b7 Compare November 1, 2017 17:09

petebacondarwin force-pushed the suspend-scope branch 2 times, most recently from cdda529 to cee0dc0 Compare November 1, 2017 18:18

poshest reviewed Nov 2, 2017

View reviewed changes

petebacondarwin force-pushed the suspend-scope branch from cee0dc0 to 65360d5 Compare November 3, 2017 11:06

gkalpak suggested changes Nov 3, 2017

View reviewed changes

poshest reviewed Nov 4, 2017

View reviewed changes

poshest reviewed Nov 14, 2017

View reviewed changes

petebacondarwin force-pushed the suspend-scope branch from 65360d5 to 701aeeb Compare November 21, 2017 12:32

gkalpak approved these changes Nov 22, 2017

View reviewed changes

petebacondarwin force-pushed the suspend-scope branch from 701aeeb to f1d6c0e Compare November 22, 2017 16:52

petebacondarwin force-pushed the suspend-scope branch from f1d6c0e to ba6874b Compare December 11, 2017 19:47

petebacondarwin merged commit 41d5c90 into angular:master Dec 11, 2017

mgol mentioned this pull request Apr 11, 2018

update changelog with 1.6.10 and 1.7.0-rc.0 #16528

Merged

3 tasks

feat($rootScope): allow suspending and resuming watchers on scope #16308

feat($rootScope): allow suspending and resuming watchers on scope #16308

Conversation

petebacondarwin commented Oct 31, 2017

googlebot commented Oct 31, 2017

petebacondarwin commented Oct 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poshest Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz Nov 1, 2017 • edited Loading

Choose a reason for hiding this comment

petebacondarwin commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poshest commented Nov 2, 2017

gkalpak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkalpak Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petebacondarwin commented Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poshest Nov 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Narretz Nov 7, 2017 • edited Loading

Choose a reason for hiding this comment

poshest commented Nov 4, 2017 • edited Loading

petebacondarwin commented Nov 4, 2017

poshest commented Nov 4, 2017

petebacondarwin commented Nov 4, 2017 • edited Loading

poshest commented Nov 4, 2017

Narretz commented Nov 6, 2017

petebacondarwin commented Nov 13, 2017

petebacondarwin commented Nov 13, 2017

Narretz commented Nov 13, 2017

petebacondarwin commented Nov 13, 2017

poshest commented Nov 14, 2017

petebacondarwin commented Nov 14, 2017

Choose a reason for hiding this comment

poshest commented Nov 14, 2017

Narretz commented Nov 14, 2017

poshest commented Nov 14, 2017

petebacondarwin commented Nov 14, 2017 • edited Loading

gkalpak commented Nov 14, 2017

poshest Nov 2, 2017 •

edited

Loading

Narretz Nov 1, 2017 •

edited

Loading

petebacondarwin Nov 21, 2017 •

edited

Loading

gkalpak Nov 3, 2017 •

edited

Loading

petebacondarwin commented Nov 3, 2017 •

edited

Loading

poshest Nov 4, 2017 •

edited

Loading

Narretz Nov 7, 2017 •

edited

Loading

poshest commented Nov 4, 2017 •

edited

Loading

petebacondarwin commented Nov 4, 2017 •

edited

Loading

petebacondarwin commented Nov 14, 2017 •

edited

Loading

petebacondarwin commented Nov 17, 2017 •

edited

Loading

Narretz commented Nov 17, 2017 •

edited

Loading

petebacondarwin commented Nov 27, 2017 •

edited

Loading