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

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

Closed
wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
abc56d5
feat($compile): add $onDestroy directive lifecycle hook
Feb 24, 2016
1db0801
feat($compile): add $onDestroy directive lifecycle hook
Feb 25, 2016
341c7ec
feat($compile): add $onDestroy directive lifecycle hook
Feb 25, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/ng/compile.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,8 @@
* * `$onInit` - Called on each controller after all the controllers on an element have been constructed and
* had their bindings initialized (and before the pre & post linking functions for the directives on
* this element). This is a good place to put initialization code for your controller.
* * `$onDestroy` - Called when the controller's scope has been destroyed. This is a good place to put
* code for tearing down your controller and for example removing event listeners.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also be explicit about when this is called wrt to child component/directives.
(And have tests for it 😃)

*
* #### `require`
* Require another directive and inject its controller as the fourth argument to the linking function. The
Expand Down Expand Up @@ -2424,6 +2426,9 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
if (isFunction(controller.instance.$onInit)) {
controller.instance.$onInit();
}
if (isFunction(controller.instance.$onDestroy)) {
scope.$on('$destroy', controller.instance.$onDestroy);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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?
  2. 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

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your input. I fixed you second point.

}
});

// PRELINKING
Expand Down
50 changes: 49 additions & 1 deletion test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4584,6 +4584,7 @@ describe('$compile', function() {
"class Foo {\n" +
" constructor($scope) {}\n" +
" $onInit() { this.check(); }\n" +
" $onDestroy() {}\n" +
" check() {\n" +
" expect(this.data).toEqualData({\n" +
" 'foo': 'bar',\n" +
Expand All @@ -4599,6 +4600,7 @@ describe('$compile', function() {
" }\n" +
"}");
spyOn(Controller.prototype, '$onInit').andCallThrough();
spyOn(Controller.prototype, '$onDestroy').andCallThrough();

module(function($compileProvider) {
$compileProvider.directive('fooDir', valueFn({
Expand All @@ -4625,7 +4627,11 @@ describe('$compile', function() {
'dir-str="Hello, {{whom}}!" ' +
'dir-fn="fn()"></div>')($rootScope);
expect(Controller.prototype.$onInit).toHaveBeenCalled();
expect(Controller.prototype.$onDestroy).not.toHaveBeenCalled();
expect(controllerCalled).toBe(true);
$rootScope.$destroy();
expect(Controller.prototype.$onDestroy).toHaveBeenCalled();

});
/*jshint +W061 */
});
Expand Down Expand Up @@ -5351,6 +5357,29 @@ describe('$compile', function() {
});
});

it('should call `controller.$onDestroy`, if provided when the controllers is destroyed', function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

controllers --> controller
(although, technically, the controller is not destroyed.)


function Controller1($element) { this.id = 1; this.element = $element; }
Controller1.prototype.$onDestroy = function() {};
spyOn(Controller1.prototype, '$onDestroy').andCallThrough();

function Controller2($element) { this.id = 2; this.element = $element; }
Controller2.prototype.$onDestroy = function() {};
spyOn(Controller2.prototype, '$onDestroy').andCallThrough();

angular.module('my', [])
.directive('d1', valueFn({ controller: Controller1 }))
.directive('d2', valueFn({ controller: Controller2 }));

module('my');
inject(function($compile, $rootScope) {
element = $compile('<div d1 d2></div>')($rootScope);
$rootScope.$destroy();
expect(Controller1.prototype.$onDestroy).toHaveBeenCalledOnce();
expect(Controller2.prototype.$onDestroy).toHaveBeenCalledOnce();
});
});

describe('should not overwrite @-bound property each digest when not present', function() {
it('when creating new scope', function() {
module(function($compileProvider) {
Expand Down Expand Up @@ -5741,6 +5770,8 @@ describe('$compile', function() {
siblingController = this.friend;
};
spyOn(MeController.prototype, '$onInit').andCallThrough();
MeController.prototype.$onDestroy = function() {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Place this line right under MeController.prototype.$onInit = ....

spyOn(MeController.prototype, '$onDestroy').andCallThrough();

angular.module('my', [])
.directive('me', function() {
Expand Down Expand Up @@ -5770,8 +5801,12 @@ describe('$compile', function() {
inject(function($compile, $rootScope, meDirective) {
element = $compile('<parent><me sibling></me></parent>')($rootScope);
expect(MeController.prototype.$onInit).toHaveBeenCalled();
expect(MeController.prototype.$onDestroy).not.toHaveBeenCalled();
expect(parentController).toEqual(jasmine.any(ParentController));
expect(siblingController).toEqual(jasmine.any(SiblingController));
$rootScope.$destroy();
expect(MeController.prototype.$onDestroy).toHaveBeenCalled();

});
});

Expand All @@ -5788,6 +5823,9 @@ describe('$compile', function() {
};
spyOn(MeController.prototype, '$onInit').andCallThrough();

MeController.prototype.$onDestroy = function() {};
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Place this line right under MeController.prototype.$onInit = ....

spyOn(MeController.prototype, '$onDestroy').andCallThrough();

angular.module('my', [])
.directive('me', function() {
return {
Expand All @@ -5814,8 +5852,12 @@ describe('$compile', function() {
inject(function($compile, $rootScope, meDirective) {
element = $compile('<parent><me sibling></me></parent>')($rootScope);
expect(MeController.prototype.$onInit).toHaveBeenCalled();
expect(MeController.prototype.$onDestroy).not.toHaveBeenCalled();
expect(parentController).toBeUndefined();
expect(siblingController).toBeUndefined();
$rootScope.$destroy();
expect(MeController.prototype.$onDestroy).toHaveBeenCalled();

});
});

Expand All @@ -5830,9 +5872,12 @@ describe('$compile', function() {
$onInit: function() {
parentController = this.container;
siblingController = this.friend;
}
},
$onDestroy: function() {}
};
spyOn(meController, '$onInit').andCallThrough();
spyOn(meController, '$onDestroy').andCallThrough();

return meController;
}

Expand Down Expand Up @@ -5864,8 +5909,11 @@ describe('$compile', function() {
inject(function($compile, $rootScope, meDirective) {
element = $compile('<parent><me sibling></me></parent>')($rootScope);
expect(meController.$onInit).toHaveBeenCalled();
expect(meController.$onDestroy).not.toHaveBeenCalled();
expect(parentController).toEqual(jasmine.any(ParentController));
expect(siblingController).toEqual(jasmine.any(SiblingController));
$rootScope.$destroy();
expect(meController.$onDestroy).toHaveBeenCalled();
});
});

Expand Down