Skip to content

Commit

Permalink
refactor($scope): prevent multiple calls to listener on $destroy
Browse files Browse the repository at this point in the history 
Prevent isolated scopes from having listeners that get called
multiple times when on `$destroy`
  • Loading branch information
@lgalfaso
lgalfaso committed Sep 15, 2014
1 parent 5cf1d89 commit 47f6d46
Show file tree
Hide file tree
Showing 2 changed files with 8 additions and 0 deletions.
1 change: 1 addition & 0 deletions src/ng/rootScope.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,7 @@ function $RootScopeProvider(){
// ensure that there is just one async queue per $rootScope and its children
child.$$asyncQueue = this.$$asyncQueue;
child.$$postDigestQueue = this.$$postDigestQueue;
child.$on('$destroy', function() { child.$$destroyed = true; });

This comment has been minimized.

Sign in to view

Copy link
@petebacondarwin

petebacondarwin Sep 26, 2014

It worries me that destroying a parent scope doesn't actually destroy child scope already! I guess this was because before angular#9281 lands, it is likely that transclude scopes are not properly contained by their template's scope and so we have been relying on DOM removal to trigger destruction of most child scopes.

This change means that if a parent scope is destroyed, it is no longer possible to call $destroy on child scopes, which in turn means that we cannot clean up listeners on those scopes.

I angular#9281 lands, we should, instead of this recurse down the scope tree when a scope is destroyed and explicitly call $destroy on each descendant.

This comment has been minimized.

Sign in to view

Copy link
@lgalfaso

lgalfaso Sep 27, 2014

Author Owner

@petebacondarwin Even after 6417a3e, nothing special is being done on $destroy. I would guess that if something special needs to be done when destroying scopes that have a parent that is different that the scope they inherit from, then these scopes would need to listen to the $destroy event and perform any special cleanup.

That said, even with the new transclude implementation, I am not sure how this would be a problem

This comment has been minimized.

Sign in to view

Copy link
@petebacondarwin

petebacondarwin Sep 28, 2014

@lgalfaso - 6417a3e doesn't do anything about scope destruction, I agree. But what it does do is make all scopes have a sensible life cycle based on their containing scope. Previously transclude scopes were "owned" by the scope from which they originated; but this only makes sense if you don't have nested transclusion, since the isolated (template's) scope and the transclusion scope were siblings. Once you have nested transclusion the transclusion scope end up being some kind of great uncle and so its lifecycle becomes very different.

If we have three directives (directive-1, directive-2 and directive-3) that all allow transclusion and have templates that look like:

<div>
  <h2>Directive X</h2>
  <div ng-transclude></div>
</div>

Then if we compiled the following:

<directive-1>
  <directive-2>
    <directive-3>
      <div>some content to transclude</div>
    </directive-3>
  </directive-2>
</directive-1>

Previously the scope hierarchy looked like:

root
  - transclude
  - isolate 1
    - isolate 2
      - isolate 3

So if directive-1 decided to destroy its content (and so its scope) it would naturally disconnect isolate-2 and isolate-3 from the scope tree but not transclude. We relied upon the DOM $destroy event to tell us that isolate-2, isolate-3 and transclude were to be destroyed.

Even before 6417a3e the fix suggested in this PR would mean that the DOM $destroy event on isolate-3 and transclude would not cause those scopes to be destroyed properly since they would have been flagged as destroyed already by the destruction of isolate-2.

After 6417a3e these scope are now in the following hierarchy:

root
  - isolate 1
    - isolate 2
      - isolate 3
        - transclude

which makes sense as the transcluded content is a child of directive-3's template and so is "owned" by the isolate-3 scope (while inheriting its properties from only root).

My suggestion is that now that we have a better ownership hierarchy of scopes we can simply recurse down the tree calling $destroy() directly on all descendents when a parent is destroyed, knowing that we will correctly destroy all the child scopes (including the transclusion scope) without having to rely on any scope $destroy events. This leaves $destroy to be used solely by applications who have attached additional resources to a scope that need to be removed when the scope is destroyed.

This comment has been minimized.

Sign in to view

Copy link
@lgalfaso

lgalfaso Sep 28, 2014

Author Owner

@petebacondarwin ok, now I understand what you are talking about.
The reason why this patch reused the events handling and only attached to isolated scopes is for performance reasons as if you do not have isolated scopes, then the on destroy we do not have to traverse the entire scope tree (and if there are a few isolated scopes, any of the sub-branches that do not would not be traversed)

Now, I do see that if parent and this are not the same, then we can run into issues when isolate 3 is destroyed and root is still valid as transclude would not know that it was actually destroyed

root
  - isolate 1
    - isolate 2
      - isolate 3
        - transclude

Instead of having to traverse the entire tree, I would find it better that for non-isolated scopes that if parent is not equals to this then we add the same listener as it is the case for isolated scope. I.e.
if (parent !== this) child.$on('$destroy', function() { child.$$destroyed = true; });

Will update the patch with this change
} else {
// Only create a child scope class if somebody asks for one,
// but cache it to allow the VM to optimize lookups.
Expand Down
7 changes: 7 additions & 0 deletions test/ng/rootScopeSpec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -994,6 +994,13 @@ describe('Scope', function() {
expect(log).toBe('123');
}));

it('should broadcast the $destroy only once', inject(function($rootScope, log) {
var isolateScope = first.$new(true);
isolateScope.$on('$destroy', log.fn('event'));
first.$destroy();
isolateScope.$destroy();
expect(log).toEqual('event');
}));

it('should decrement ancestor $$listenerCount entries', inject(function($rootScope) {
var EVENT = 'fooEvent',
Expand Down

0 comments on commit 47f6d46

Please sign in to comment.