Pr 14656 #14811

petebacondarwin · 2016-06-21T09:51:05Z

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

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

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

Does this PR introduce a breaking change?

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:

googlebot · 2016-06-21T09:51:06Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

googlebot · 2016-06-21T09:51:06Z

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

petebacondarwin · 2016-06-21T09:53:33Z

src/ng/compile.js

@@ -2499,6 +2538,10 @@ function $CompileProvider($provide, $$sanitizeUriProvider) {
              $exceptionHandler(e);
            }
          }
+          if (isFunction(controllerInstance.$doCheck)) {
+            controllerScope.$watch(function() { controllerInstance.$doCheck(); });


we don't need to manually track the remove watch function because we are after the point when we might swap out the controller for another instance. This watch will naturally be removed when the controller scope is destroyed.

gkalpak · 2016-06-21T11:13:07Z

One thing to note:

In Angular 2, there is no "digest loop" (running until the values have stabilized, which means at least twice per digest cycle) and the change detection is "single pass". Since the change detection works differently in Angular 1, there can't be a 1-to-1 mapping of the behavior of DoCheck between the two.

In particular, the ng2 behavior could be mapped to two strategies in ng1:

We run $doCheck() on every digest loop iteration (i.e. multiple times per digest cycle).
We run $doCheck() as a post-digest task (i.e. once after the digest cycle is over).
(This would be similar to how $onChanges() is implemented, btw.)

This PR uses strategy (1), so currently the order of execution when there is a change is: $doCheck --> $onChanges --> $doCheck. In ng2, it will be $onChanges --> $doCheck (see demo).
It is not 100% consistent, but it is as consistent as we can get (within reason).

What I am sceptical about, is that with the current ng1 implementation it will be possible to do changes that will be picked up in the current digest cycle and apps will work fine. In ng2, this won't be the case.
You will either get exceptions (in dev mode) or the UI won't reflect the changes initiated through ngDoCheck() (see other demo - comment enableProdMode(); in app/main.ts to change mode).

In that sense, it might be more consistent if we implemented strategy (2) without invoking another digest.

_{I am more like thinking load than proposing anything tbh...}

petebacondarwin · 2016-06-21T11:25:54Z

Although it is worth noting that in A2 it is not recommended to use both ngDoCheck and ngOnChanges together.

gkalpak · 2016-06-21T11:47:23Z

To be clear, my concern is not about $onChanges, it's about the view not being updated with changes initiated from ngDoCheck (in ng2).

petebacondarwin · 2016-06-21T11:57:05Z

As an aside here are two demos of A2 ngDoChanges:

Date http://plnkr.co/edit/1d1rlm2efCP31MphBvsf?p=preview
Arrays http://plnkr.co/edit/ZssO7amzxBIg3WL5W1at?p=preview

petebacondarwin · 2016-06-21T11:57:31Z

@gkalpak can you put a unit test (or demo) together to illustrate your concern in A1?

petebacondarwin · 2016-06-21T11:59:36Z

Oh, I just disabled Prod Mode and see what you now mean...

petebacondarwin · 2016-06-21T12:01:05Z

But even if we put the changes in $$postDigest we will still have a problem right? SInce in dev mode it errors and in ng1 it would just do nothing (or wait till the next digest)

mazzur · 2016-06-21T12:19:36Z

@gkalpak as far as I understood the main constraint is that we should not be able to trigger changes outside the component in its doCheck hook (as it is in A2) while changes inside the component should work just as it's implemented in this request. Please correct me if I'm wrong.

mazzur · 2016-06-21T12:55:21Z

But even if we put the changes in $$postDigest we will still have a problem right? Since in dev mode it errors and in ng1 it would just do nothing (or wait till the next digest)

We can trigger digest on the component scope following the $doCheck call in $$postDigest so that only values from inside the component would be applied right away as it seems to be in A2.

petebacondarwin · 2016-06-21T13:13:21Z

We can trigger digest on the component scope following the $doCheck call in $$postDigest so that only values from inside the component would be applied right away as it seems to be in A2.

Partial digesting is a worrying topic...

petebacondarwin · 2016-06-21T13:14:44Z

@mazzur I think you are correct in your assumption that we should not trigger changes outside - but I don't think that we can honour this in Angular 1 without doing dastardly stuff, like partial digests

zbjornson · 2016-06-21T17:29:18Z

test/ng/compileSpec.js

@@ -3872,6 +3872,51 @@ describe('$compile', function() {
          ]);
        });
      });
+
+      it('should work if $doCheck is provided in the constructor', function() {


This test looks identical to the one I added above?

The test above assigns the methods on the prototype. This test defines them on the instance from inside the constructor.
(Note that while both work in ng1, in ng2 only the prototype will work.)

Aha okay, thanks for explaining :)

petebacondarwin · 2016-06-21T17:54:50Z

So I think that we should go with this implementation but document the following divergences from Angular 2:

in Angular 2 you cannot define the ngDoCheck method inside the constructor, it must be on the prototype
in Angular 2 you must not use the ngDoCheck method to trigger a change outside of the component's template.
in Angular 1 you may get many more calls to $doCheck than you would to ngDoCheck in Angular 2 due to the differences in the change detection mechanism

gkalpak · 2016-06-21T18:16:27Z

Except that if we document the 1st deviation (not being on the prototype), we should document it for all lifecycle hooks.

Backuport ngDoCheck from Angular 2.

…er constructor

Closes #14811

…er constructor Closes #14811

Closes #14811

googlebot added the cla: no label Jun 21, 2016

petebacondarwin mentioned this pull request Jun 21, 2016

feat($compile): backport $doCheck #14656

Closed

3 tasks

petebacondarwin added type: feature component: $compile needs: review needs: public api branch: 1.5.x labels Jun 21, 2016

petebacondarwin added this to the 1.5.8 milestone Jun 21, 2016

petebacondarwin reviewed Jun 21, 2016
View reviewed changes

petebacondarwin force-pushed the pr-14656 branch from f82d100 to 453d74f Compare June 21, 2016 10:58

zbjornson reviewed Jun 21, 2016
View reviewed changes

zbjornson and others added 5 commits June 22, 2016 10:31

feat($compile): backport $doCheck

8f63c5a

Backuport ngDoCheck from Angular 2.

fix($compile): ensure $doCheck hooks can be defined in the controll…

5096e34

…er constructor

docs($compile): add additional runnable examples for the $doCheck hook

b409bed

docs($compile): reorganize the life-cycle hooks docs

69be7a9

docs($http): fix a dangling link

2dcbc4e

petebacondarwin force-pushed the pr-14656 branch from de17903 to 2dcbc4e Compare June 22, 2016 09:32

petebacondarwin closed this in 4c662e1 Jun 22, 2016

petebacondarwin added a commit that referenced this pull request Jun 22, 2016

docs($compile): add additional runnable examples for the $doCheck hook

e62e59f

Closes #14811

petebacondarwin added a commit that referenced this pull request Jun 22, 2016

docs($compile): reorganize the life-cycle hooks docs

b94626c

Closes #14811

petebacondarwin added a commit that referenced this pull request Jun 22, 2016

fix($compile): ensure $doCheck hooks can be defined in the controll…

3010ed4

…er constructor Closes #14811

petebacondarwin added a commit that referenced this pull request Jun 22, 2016

docs($compile): add additional runnable examples for the $doCheck hook

fd57df1

Closes #14811

petebacondarwin added a commit that referenced this pull request Jun 22, 2016

docs($compile): reorganize the life-cycle hooks docs

2ca4170

Closes #14811

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pr 14656 #14811

Pr 14656 #14811

petebacondarwin commented Jun 21, 2016

googlebot commented Jun 21, 2016

googlebot commented Jun 21, 2016

petebacondarwin Jun 21, 2016

gkalpak commented Jun 21, 2016

petebacondarwin commented Jun 21, 2016

gkalpak commented Jun 21, 2016

petebacondarwin commented Jun 21, 2016

petebacondarwin commented Jun 21, 2016

petebacondarwin commented Jun 21, 2016 •

edited

Loading

petebacondarwin commented Jun 21, 2016

mazzur commented Jun 21, 2016 •

edited

Loading

mazzur commented Jun 21, 2016 •

edited

Loading

petebacondarwin commented Jun 21, 2016

petebacondarwin commented Jun 21, 2016

zbjornson Jun 21, 2016

gkalpak Jun 21, 2016

zbjornson Jun 21, 2016

petebacondarwin commented Jun 21, 2016

gkalpak commented Jun 21, 2016

Pr 14656 #14811

Pr 14656 #14811

Conversation

petebacondarwin commented Jun 21, 2016

googlebot commented Jun 21, 2016

googlebot commented Jun 21, 2016

petebacondarwin Jun 21, 2016

Choose a reason for hiding this comment

gkalpak commented Jun 21, 2016

petebacondarwin commented Jun 21, 2016

gkalpak commented Jun 21, 2016

petebacondarwin commented Jun 21, 2016

petebacondarwin commented Jun 21, 2016

petebacondarwin commented Jun 21, 2016 • edited Loading

petebacondarwin commented Jun 21, 2016

mazzur commented Jun 21, 2016 • edited Loading

mazzur commented Jun 21, 2016 • edited Loading

petebacondarwin commented Jun 21, 2016

petebacondarwin commented Jun 21, 2016

zbjornson Jun 21, 2016

Choose a reason for hiding this comment

gkalpak Jun 21, 2016

Choose a reason for hiding this comment

zbjornson Jun 21, 2016

Choose a reason for hiding this comment

petebacondarwin commented Jun 21, 2016

gkalpak commented Jun 21, 2016

petebacondarwin commented Jun 21, 2016 •

edited

Loading

mazzur commented Jun 21, 2016 •

edited

Loading

mazzur commented Jun 21, 2016 •

edited

Loading