feat($compile): add $onDestroy directive lifecycle hook by bobbijvoet · Pull Request #14127 · angular/angular.js

bobbijvoet · 2016-02-24T17:49:01Z

This PR introduces a new $onDestroy lifecycle hook for directives. This hook matches the destroy hook that's present in Angular 2.

Closes #14020

Introduce a destroy hook for directives which follows the ng2 component lifecycle Closes angular#14020

dcherman · 2016-02-25T06:56:40Z

src/ng/compile.js

            controller.instance.$onInit();
          }
+          if (isFunction(controller.instance.$onDestroy)) {
+            scope.$on('$destroy', controller.instance.$onDestroy);


Two main comments here (was actually implementing this just now until I saw this):

This doesn't allow late-bound $onDestroy methods to be added. If it's not there at the time of controller instantiation, the lifecycle hook will not function. Does that follow expectations, or do we need to be checking this at the time of destruction?

The this binding in the $onDestroy method is going to be incorrect here due to the manner in which you're passing $onDestroy. I see that you copied the unit tests from the one above it as I had done, but removed the check call. You should re-introduce that portion of the test since it would have failed on this point

Thanks for your input. I fixed you second point.

Bind $onDestroy handler to the controller instance Closes angular#14020

bobbijvoet · 2016-02-25T08:26:48Z

@dcherman Yes, we should allow late-bound $onDestroy methods. We could attach scope.$on('$destroy') listeners for every controller and then check if the $onDestroy method is there. But it does not feel right to attach this listener on every controller. What do you think?

gkalpak · 2016-02-25T09:08:05Z

src/ng/compile.js

            controller.instance.$onInit();
          }
+          if (isFunction(controller.instance.$onDestroy)) {
+            scope.$on('$destroy', controller.instance.$onDestroy.bind(controller.instance));


The problem with this is that scope might be a parent scope that is not directly related to the element.
Maybe we could use $element.on('$destroy', ...) instead (I haven't given it much thought, though).

Ok. The element.$on solution sounds better to me because we care about the destruction of the component/directive in specific, not about the scope. Thanks for the insight, will create a new solution.

Call $onDestroy when element is removed, possibility to set $onDestroy method after controller initialization Closes angular#14020

dcherman · 2016-02-25T16:10:03Z

src/ng/compile.js

            controller.instance.$onInit();
          }
+
+          $element.on('$destroy', function() {


The $destroy event won't function properly when you have a transclude: "element" directive since events are not fired on the comment node that becomes $element.

http://jsfiddle.net/rogqp47y/17/

So it should be implemented using $scope.$on('$destroy', ... ?

dcherman · 2016-02-25T16:31:46Z

Figuring out exactly where to run this logic is trickier than it looks. We only have two existing hooks that can be used for something like this which are the $destroy events fired on both the scope and element. Both events have disadvantages that make them non-functional in some scenarios which puts us in a tight spot here. I haven't thought of an alternative solution yet.

dcherman · 2016-03-01T17:41:04Z

Edit: This idea is pretty much invalid/derp. The new scope's lifecycle would match the parent's, making it pointless to have created it in the first place.

How common do we feel that transclude: 'element' or restrict: "M" directives with controllers that do not specify a new scope are? Uncommon enough to add an assertion for invalid usage maybe? If we can manage $onDestroy hooks via scopes for comment based things and the elements for others, then this seems more possible.

So I was thinking about this for a little bit this morning, and although the $destroy event on the element is close to what we want, it simply won't work for comment nodes. There's a variety of methods I can think of to kind of emulate that, but none of them are synchronous and match the behavior of the $destroy event, and every idea probably has some horrible performance implications as well. There's no real performant way of saying "get all descendant comment nodes", so modifying cleanData to work on comment nodes is pretty much a non-starter as well.

I have a potentially simpler idea though. Right now, the scope hierarchy is almost entirely managed by higher level scopes; there's no implicit dependency on the $destroy element event being invoked to cleanup the hierarchy. As a result, that means that anyone that wishes to do anything that results in scope cleanup must already be calling $destroy on themselves (including internals like ngRepeat and ngIf.

~~With that in mind, what if we were to detect that no scope was requested for the current set of elements, but controllers exist that may potentially contain $onDestroy methods?~~

This shouldn't affect the user's scope hierarchy since it's a sibling to any existing scopes, similar to a transcluded scope. You could even expand on this solution and only do this for directives that have a comment node as the root element, although I'd be concerned about potential minor differences between the element lifecycle and the scope one, mainly in the ordering of when the $onDestroy methods would be invoked.

I have no data to back up this assertion (at all), but I feel like it may not be that common to have a controller on a directive that doesn't define either an isolate or new scope, so this codepath may be hit less frequently than we might imagine. The new component helper event reinforces that idea by requiring an isolate scope be used.

…t is removed

gkalpak · 2016-03-05T13:16:06Z

The more I think about it, the more it doesn't make sense to try to cover all corner-cases and end up with a complicated bloat (that will have little resemblance to ng2).

Since the main purpose (or at least one of the main purposes) of the directive lifecycle hooks is to enable structuring your directive in a way that is close to the ng2-way (and thus facilitate the migration), it is reasonable to only offer the $onDestroy hook on directives that define their own scope.

In ng2 all directives have sort of their own scope/context (the component/directive class). So, by not providing the $onDestroy() hook on directives with scope: false, we won't be hurting the migration (since those directives are not "migratable" anyway).

This way, we will avoid lots of irrelevant corner cases and we'll be able to implement it using $scope.$on('$destroy', ...), which is straight-forward and safe.

WDYT ?

/cc @petebacondarwin

drpicox · 2016-03-05T17:16:32Z

I start to think that may be components are not directives:
(in the same way that squares does not inherit rectangles, ¿how do you change height but not width and still being a square?)

they have always isolated scope,
they are always restrict to entity,
always have controller (with bindings),
templates does not have namespace,
no multielement, no link, ...
there cannot be two components in the same element (E with controller)

So, may be hooks can only be applied from component constructor or only to components. In such case no need for forEach controller, less overhead in directives, ... (may be all of this can be achieved by providing pre/post-link functions just for components).

petebacondarwin · 2016-03-05T20:57:58Z

I think we should implement it via $scope.$on('$destroy', ...) for all directives that have a controller which has a $onDestroy method. If they are not creating their own scope then they will just have to wait for their containing scope to be destroyed, which should really map exactly to when their element is destroyed.

drpicox · 2016-03-05T22:02:27Z

What I mean about components "are not" directives is may be that something such simple as changing compile.js to add pre-post link like that:

      return {
        controller: controller,
        controllerAs: identifierForController(options.controller) || options.controllerAs || '$ctrl',
        template: makeInjectable(template),
        templateUrl: makeInjectable(options.templateUrl),
        transclude: options.transclude,
        scope: {},
        bindToController: options.bindings || {},
        restrict: 'E',
        require: !options.require && [name] || [name].concat(options.require),
        link: {
          pre: function(scope, element, attrs, controllers) {
            if (isFunction(controllers[0].$onInit)) {
             controllers[0].$onInit();
            }
          },
          post: function(scope, element, attrs, controllers) {
            scope.$on('$destroy', function() {
              if (isFunction(controllers[0].$onDestroy)) {
               controllers[0].$onDestroy();
              }
            });
            // placed after registering to avoid not firing destroy if afterViewInit throws an exception (and save a non-optimizable try-catch)
            if (isFunction(controllers[0].$afterViewInit)) {
             controllers[0].$afterViewInit();
            }
          },   
      };

petebacondarwin · 2016-03-06T08:15:33Z

@drpicox - I like what you are saying but I feel that developers ought to expect the same hooks to be called even if they create component directives using the mod.directive() helper - as long as their directive looks like a component.

drpicox · 2016-03-06T14:13:52Z

Yes, you are right. I was just trying to give another vision, but right, it is not the best one.

…ngular#14127)

This change adds in the following new lifecycle hooks, which map in some way to those in Angular 2: * `$onChanges(changesObj)` - Called whenever one-way bindings are updated. The `changesObj` is a hash whose keys are the names of the bound properties that have changed, and the values are an object of the form `{ currentValue: ..., previousValue: ... }`. Use this hook to trigger updates within a component such as cloning the bound value to prevent accidental mutation of the outer value. * `$onDestroy` - Called on a controller when its containing scope is destroyed. Use this hook for releasing external resources, watches and event handlers. * `$afterViewInit` - Called after this controller's element and its children been linked. Similar to the post-link function this hook can be used to set up DOM event handlers and do direct DOM manipulation. Note that child elements that contain `templateUrl` directives will not have been compiled and linked since they are waiting for their template to load asynchronously and their own compilation and linking has been suspended until that occurs. Closes angular#14127 Closes angular#14030 Closes angular#14020 Closes angular#13991

This change adds in the following new lifecycle hooks, which map in some way to those in Angular 2: * `$onChanges(changesObj)` - Called whenever one-way bindings are updated. The `changesObj` is a hash whose keys are the names of the bound properties that have changed, and the values are an object of the form `{ currentValue: ..., previousValue: ... }`. Use this hook to trigger updates within a component such as cloning the bound value to prevent accidental mutation of the outer value. * `$onDestroy` - Called on a controller when its containing scope is destroyed. Use this hook for releasing external resources, watches and event handlers. * `$afterViewInit` - Called after this controller's element and its children been linked. Similar to the post-link function this hook can be used to set up DOM event handlers and do direct DOM manipulation. Note that child elements that contain `templateUrl` directives will not have been compiled and linked since they are waiting for their template to load asynchronously and their own compilation and linking has been suspended until that occurs. Closes angular#14127 Closes angular#14030 Closes angular#14020 Closes angular#13991 Closes angular#14302

This change adds in the following new lifecycle hooks, which map in some way to those in Angular 2: * `$onChanges(changesObj)` - Called whenever one-way bindings are updated. The `changesObj` is a hash whose keys are the names of the bound properties that have changed, and the values are an object of the form `{ currentValue: ..., previousValue: ... }`. Use this hook to trigger updates within a component such as cloning the bound value to prevent accidental mutation of the outer value. * `$onDestroy` - Called on a controller when its containing scope is destroyed. Use this hook for releasing external resources, watches and event handlers. * `$postLink` - Called after this controller's element and its children been linked. Similar to the post-link function this hook can be used to set up DOM event handlers and do direct DOM manipulation. Note that child elements that contain `templateUrl` directives will not have been compiled and linked since they are waiting for their template to load asynchronously and their own compilation and linking has been suspended until that occurs. Closes angular#14127 Closes angular#14030 Closes angular#14020 Closes angular#13991 Closes angular#14302

This change adds in the following new lifecycle hooks, which map in some way to those in Angular 2: * `$onChanges(changesObj)` - Called whenever one-way bindings are updated. The `changesObj` is a hash whose keys are the names of the bound properties that have changed, and the values are an object of the form `{ currentValue: ..., previousValue: ... }`. Use this hook to trigger updates within a component such as cloning the bound value to prevent accidental mutation of the outer value. * `$onDestroy` - Called on a controller when its containing scope is destroyed. Use this hook for releasing external resources, watches and event handlers. * `$postLink` - Called after this controller's element and its children been linked. Similar to the post-link function this hook can be used to set up DOM event handlers and do direct DOM manipulation. Note that child elements that contain `templateUrl` directives will not have been compiled and linked since they are waiting for their template to load asynchronously and their own compilation and linking has been suspended until that occurs. Closes #14127 Closes #14030 Closes #14020 Closes #13991 Closes #14302

feat($compile): add $onDestroy directive lifecycle hook

abc56d5

Introduce a destroy hook for directives which follows the ng2 component lifecycle Closes angular#14020

googlebot added the cla: yes label Feb 24, 2016

dcherman reviewed Feb 25, 2016
View reviewed changes

feat($compile): add $onDestroy directive lifecycle hook

1db0801

Bind $onDestroy handler to the controller instance Closes angular#14020

gkalpak reviewed Feb 25, 2016
View reviewed changes

feat($compile): add $onDestroy directive lifecycle hook

341c7ec

Call $onDestroy when element is removed, possibility to set $onDestroy method after controller initialization Closes angular#14020

dcherman reviewed Feb 25, 2016
View reviewed changes

Narretz added type: feature component: $compile labels Feb 27, 2016

Narretz added this to the 1.5.x milestone Feb 27, 2016

Narretz added the needs: work label Feb 27, 2016

dcherman referenced this pull request in drpicox/angular.js Mar 4, 2016

fix(asDirectiveSpec): nullify the assigned controller when the elemen…

e8bf846

…t is removed

dcherman mentioned this pull request Mar 6, 2016

feat(ngAsDirective): new as directive to publish component controllers into current scope #14080

Closed

3 tasks

drpicox added a commit to drpicox/angular.js that referenced this pull request Mar 6, 2016

feat(asDirective): match $destroy behaviour with ctrl.$onDestroy (see a…

e25d8a9

…ngular#14127)

petebacondarwin mentioned this pull request Mar 22, 2016

feat($compile): add more lifecycle hooks to directive controllers #14302

Merged

3 tasks

petebacondarwin closed this in #14302 Mar 25, 2016

Conversation

bobbijvoet commented Feb 24, 2016

Uh oh!

dcherman Feb 25, 2016

Choose a reason for hiding this comment

Uh oh!

bobbijvoet Feb 25, 2016

Choose a reason for hiding this comment

Uh oh!

bobbijvoet commented Feb 25, 2016

Uh oh!

gkalpak Feb 25, 2016

Choose a reason for hiding this comment

Uh oh!

bobbijvoet Feb 25, 2016

Choose a reason for hiding this comment

Uh oh!

dcherman Feb 25, 2016

Choose a reason for hiding this comment

Uh oh!

drpicox Mar 5, 2016

Choose a reason for hiding this comment

Uh oh!

dcherman commented Feb 25, 2016

Uh oh!

dcherman commented Mar 1, 2016

Uh oh!

gkalpak commented Mar 5, 2016

Uh oh!

drpicox commented Mar 5, 2016

Uh oh!

petebacondarwin commented Mar 5, 2016

Uh oh!

drpicox commented Mar 5, 2016

Uh oh!

petebacondarwin commented Mar 6, 2016

Uh oh!

drpicox commented Mar 6, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants