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

petebacondarwin · 2016-03-22T18:01:34Z

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Feature

What is the current behavior? (You can also link to an open issue here)

Missing lifecycle hooks for directive controllers

What is the new behavior (if this is a feature change)?

Add these lifecycle hooks

Does this PR introduce a breaking change?

Nope

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

Other information:

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 #14127
Closes #14030
Closes #14020
Closes #13991

mgol · 2016-03-23T10:26:04Z

docs/content/guide/component.ngdoc

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


Is it also how it happens now if you use regular postLink functions in module.directive-defined directives?

Yes

_{I copied and pasted that text from the post link function :-)}

TBH, I feel that this last limitation makes this hook pretty usel...ehm...much less useful than the corresponding ng2 hook :(

I assume, with the current implementation, this hook is very similar to the post-link function, which means that:

(a) It doesn't offer new functionality for ng1 users.
(b) It doesn't help much with migrating but for very simple components with inline templates. Even caching the templates won't help, right ?

These are just assumptions atm (I haven't looked at the code yet), but if they are true, I'm a little skeptical about this hook.

In my current project I avoid these problems by not using templateUrl but relying on Webpack being able to import HTML files using the ES6 import syntax. Unfortunately you can't do things like that without any preprocessing done so it can't be a general solution.

You are right @gkalpak, which is why I was suggesting that we call it $afterLink instead.
It does, though, have value for components, where there is no way right now to hook into the post-link function.

I am becoming very keen on the webpack template: require('some.template.html') approach :-)

Yeah, I only wish TypeScript had something similar :(
But in the meantime, I believe most developers follow the "best practice" of caching their templates with $templateCache in a build step. Besides a component doesn't know what it's children (or its grandchildren) do wrt template vs templateUrl.

I am afraid this will confuse people and create some headaches while migrating.

If we can't/won't make it work similarly to ng2, it might be btter to name is $postLink and just provide it as a way to access the post-link function while using the .component() helper, as @petebacondarwin suggested.

mgol · 2016-03-23T10:40:40Z

I'd add tests that ensure $onChanges is not called on changes in other kinds of bindings.

petebacondarwin · 2016-03-23T10:55:31Z

Fixed the comment typo and added tests @mgol PTAL

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

gkalpak · 2016-03-23T11:10:15Z

docs/content/guide/component.ngdoc

+    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


been linked --> have been linked

mgol · 2016-03-23T11:18:28Z

LGTM (once the typo @gkalpak noticed is fixed)

gkalpak · 2016-03-23T11:22:01Z

src/ng/compile.js

 *   had their bindings initialized (and before the pre &amp; post linking functions for the directives on
 *   this element). This is a good place to put initialization code for your controller.
+ * * `$onChanges(changesObj)` - Called whenever one-way bindings are updated. The `changesObj` is a hash whose keys


Nitpick: I would put the < symbol there somewhere, because @ bindings are also called "one-way" in many blogsposts/tutorials and this might confuse users.

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

gkalpak · 2016-03-23T11:31:52Z

src/ng/compile.js

+
+      function triggerOnChangesHook() {
+        // We must run this hook in an apply since the $$postDigest runs outside apply
+        scope.$apply(function() {


Doesn't this mean there will be one extra digest for each controller (that has at least one changed binding) per digest ?

Yes you are right.
The alternative is to use $applyAsync but this has the downside of delaying the call to $onChanges by maybe 100ms.

What is better?

I think we could move the login that calls triggerOnChangeHook inside $compile (I mean make it a compiler-local thing) and store the changes for each controller there, so we only trigger one extra $digest.

gkalpak · 2016-03-23T12:05:43Z

I am also wondering if applying $onChange to @ bindings would be closer to ng2 (iirc it will).

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

gkalpak · 2016-03-23T14:35:15Z

docs/content/guide/component.ngdoc

@@ -147,6 +147,30 @@ components should follow a few simple conventions:
      }
    ```

+- **Components have a well-defined lifecycle


Missing closing **.

gkalpak · 2016-03-23T14:56:55Z

src/ng/compile.js

+    function flushOnChangesQueue() {
+      // We must run this hook in an apply since the $$postDigest runs outside apply
+      $rootScope.$apply(function() {
+        for (var i = 0, ii = onChangesQueue.length; i < ii; ++i) {


What about changes detected during this digest ?
We should either not hard code the length or collect them and run another digest (but then there's a risk for infinite loops).

Good call...
All the items in the onChanges queue will be flushed before the new digest is triggered.

The sequence looks like:

some change 1

some change 2

$digest

schedule flushOnChangesQueue

queue onChanges 1

queue onChanges 2

$$postDigest

flushOnChangesQueue

$apply

trigger onChanges 1

some change 1a

trigger onChanges 2

some change 2a

$digest

schedule onChangesFlush

queue onChanges 1

queue onChanges 2

$$postDigest

flushOnChangesQueue

$apply

trigger onChanges 1

trigger onChanges 2

$digest

I think this means that we are OK (except for the potential for deep recursion). I'll make a test

Added a test @gkalpak PTAL

So, this looks good apart from the infinite loop issue.

Ideally, there should be some mechanism in place to prevent freezing the whole app, because someone misused $onChanges (a TTL-like mechanism maybe).
If not (e.g. it turns out to be too complicated), we should at least document very clearly the dangers of making changes that would trigger another $onChanges round. from inside an $onChanges() callback.

It is not an infinite loop. It is an infinite recursion, which will trigger a RangeError exception that will prevent the browser from freezing and indicate to the developer that they have done a bad thing.

Added a test to demonstrate.

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

gkalpak · 2016-03-23T17:36:48Z

src/ng/compile.js

 *   had their bindings initialized (and before the pre &amp; post linking functions for the directives on
 *   this element). This is a good place to put initialization code for your controller.
+ * * `$onChanges(changesObj)` - Called whenever one-way (`<`) bindings are updated. The `changesObj` is a hash


Now we need to change this again to also mention @ 😁

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

gkalpak · 2016-03-23T18:51:29Z

LGTM with this comment (somehow) addressed
👍

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

petebacondarwin · 2016-03-24T12:06:53Z

I've added time-to-live constraints for onChanges hooks, which should address @gkalpak's last concern

Narretz · 2016-03-24T12:17:37Z

docs/content/error/$compile/infchng.ngdoc

+<c1 prop="a" on-change="a = -a"></c1>
+```
+
+```js


Can you add the component definition object? Makes it easier to understand if you know the bindings are bindings: {'prop': '<', onChange: '&'}

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

petebacondarwin added type: feature component: $compile needs: review branch: 1.5.x labels Mar 22, 2016

petebacondarwin added this to the 1.5.3 milestone Mar 22, 2016

googlebot added the cla: yes label Mar 22, 2016

mgol reviewed Mar 23, 2016
View reviewed changes

petebacondarwin force-pushed the ondestroy branch from 2afc7f3 to 5270c25 Compare March 23, 2016 10:55

gkalpak reviewed Mar 23, 2016
View reviewed changes

petebacondarwin force-pushed the ondestroy branch from 5270c25 to 8c4eef6 Compare March 23, 2016 11:25

petebacondarwin force-pushed the ondestroy branch from 8c4eef6 to 715efb8 Compare March 23, 2016 11:27

gkalpak reviewed Mar 23, 2016
View reviewed changes

petebacondarwin force-pushed the ondestroy branch from 715efb8 to cb864ce Compare March 23, 2016 13:27

petebacondarwin force-pushed the ondestroy branch from cb864ce to 5450d24 Compare March 23, 2016 14:19

gkalpak reviewed Mar 23, 2016
View reviewed changes

petebacondarwin force-pushed the ondestroy branch from 5450d24 to 1123985 Compare March 23, 2016 16:56

petebacondarwin force-pushed the ondestroy branch from 1123985 to 2266b50 Compare March 23, 2016 17:30

gkalpak reviewed Mar 23, 2016
View reviewed changes

petebacondarwin force-pushed the ondestroy branch from 2266b50 to aca8559 Compare March 23, 2016 17:57

petebacondarwin force-pushed the ondestroy branch from 2ac418e to f60d0c4 Compare March 24, 2016 09:49

Narretz reviewed Mar 24, 2016
View reviewed changes

petebacondarwin force-pushed the ondestroy branch from 7e794d3 to 874c0fd Compare March 25, 2016 12:55

petebacondarwin merged commit 874c0fd into angular:master Mar 25, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

petebacondarwin commented Mar 22, 2016

mgol Mar 23, 2016

petebacondarwin Mar 23, 2016

gkalpak Mar 23, 2016

mgol Mar 23, 2016

petebacondarwin Mar 23, 2016

petebacondarwin Mar 23, 2016

gkalpak Mar 23, 2016

mgol commented Mar 23, 2016

petebacondarwin commented Mar 23, 2016

gkalpak Mar 23, 2016

mgol commented Mar 23, 2016

gkalpak Mar 23, 2016

petebacondarwin Mar 23, 2016

gkalpak Mar 23, 2016

petebacondarwin Mar 23, 2016

gkalpak Mar 23, 2016

gkalpak commented Mar 23, 2016

gkalpak Mar 23, 2016

gkalpak Mar 23, 2016

petebacondarwin Mar 23, 2016

petebacondarwin Mar 23, 2016

gkalpak Mar 23, 2016

petebacondarwin Mar 24, 2016

gkalpak Mar 23, 2016

gkalpak commented Mar 23, 2016

petebacondarwin commented Mar 24, 2016

Narretz Mar 24, 2016

petebacondarwin Mar 24, 2016

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

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

Conversation

petebacondarwin commented Mar 22, 2016

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

mgol commented Mar 23, 2016

petebacondarwin commented Mar 23, 2016

Choose a reason for hiding this comment

mgol commented Mar 23, 2016

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

gkalpak commented Mar 23, 2016

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

gkalpak commented Mar 23, 2016

petebacondarwin commented Mar 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment