Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pr 14656 #14811

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
5 participants
@petebacondarwin
Copy link
Member

petebacondarwin commented Jun 21, 2016

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

Other information:

@googlebot

This comment has been minimized.

Copy link

googlebot commented Jun 21, 2016

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.

1 similar comment
@googlebot

This comment has been minimized.

Copy link

googlebot commented Jun 21, 2016

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.

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

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Jun 21, 2016

Author Member

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.

@@ -310,6 +316,39 @@
* they are waiting for their template to load asynchronously and their own compilation and linking has been
* suspended until that occurs.
*
* ** $doCheck example **
*
* This example show how you might use `$doCheck` to customise the equality check of component inputs.

This comment has been minimized.

Copy link
@gkalpak

gkalpak Jun 21, 2016

Member

show --> shows

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Jun 21, 2016

Author Member

thanks

*
* this.items_clone = angular.copy(this.items);
* this.items_ref = this.items;
* };

This comment has been minimized.

Copy link
@gkalpak

gkalpak Jun 21, 2016

Member

How about a simpler (and more lightweight example). We should better not encourage people to use angular.copy and angular.equals on potentially large arrays of potentially deep or large objects 😃

What happened to checking an internal field of a Date object? 😃

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Jun 21, 2016

Author Member

This was the lightest thing I could come up with but sure I could give the Date object mutation a go too. (Or are you just ribbing me?)

This comment has been minimized.

Copy link
@gkalpak

gkalpak Jun 21, 2016

Member

I was thinking something like:

<button ng-click="myDate.setMonth(myDate.getMonth() + 1)">Postpone for a month</button>
this.$doCheck = function() {
  var newValue = this.myDate.valueOf();
  if (newValue !== this.oldValue) { this.log.push('doCheck: we have a new date'); }
  this.oldValue = newValue;
};

This comment has been minimized.

Copy link
@gkalpak

gkalpak Jun 21, 2016

Member

Another usecase I have in mind for $doCheck() is for implementing a poor man's immutability-based change detection:

// In parent:
this.items = [];
this.items._version = 0;

this.addItem = function addItem(item) {
  this.items.push(item);
  this.items._version++;
};

// In child:
this.$doCheck = function $doCheck() {
  if (this.items._version !== this.oldVersion) {
    // `items` has been mutated
  }
  this.oldVersion = this.items._version;
};

(Although I am not sure this is a vaid usecase 😁)


function TestController() {
this.$doCheck = function() { log.push('$doCheck'); };
this.$onChanges = function() { log.push('$onChanges'); };

This comment has been minimized.

Copy link
@gkalpak

gkalpak Jun 21, 2016

Member

Wrong indentation

});
});

it('should work if $doCheck is provided in the constructor', function() {

This comment has been minimized.

Copy link
@gkalpak

gkalpak Jun 21, 2016

Member

I like that we have a test that verifies our incompatibility wiht ng2 😛

This comment has been minimized.

Copy link
@petebacondarwin

petebacondarwin Jun 21, 2016

Author Member

More ribbing? :-P
It is a common pattern in ng1, where classes are not the be-all-and-end-all, to put methods on the instance :-)

This comment has been minimized.

Copy link
@gkalpak

gkalpak Jun 21, 2016

Member

No ribbing intended 😃
It's a good thing that we have this test. It's not ideal that we are incompatible with ng2, but it's much better that being incompatible with ng1 users and common patterns 😉

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:pr-14656 branch from f82d100 to 453d74f Jun 21, 2016

@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Jun 21, 2016

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:

  1. We run $doCheck() on every digest loop iteration (i.e. multiple times per digest cycle).
  2. 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

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Jun 21, 2016

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

@gkalpak

This comment has been minimized.

Copy link
Member

gkalpak commented Jun 21, 2016

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

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Jun 21, 2016

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Jun 21, 2016

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

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Jun 21, 2016

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

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Jun 21, 2016

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

This comment has been minimized.

Copy link

mazzur commented Jun 21, 2016

@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

This comment has been minimized.

Copy link

mazzur commented Jun 21, 2016

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

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Jun 21, 2016

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

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Jun 21, 2016

@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

@@ -3872,6 +3872,51 @@ describe('$compile', function() {
]);
});
});

it('should work if $doCheck is provided in the constructor', function() {

This comment has been minimized.

Copy link
@zbjornson

zbjornson Jun 21, 2016

Contributor

This test looks identical to the one I added above?

This comment has been minimized.

Copy link
@gkalpak

gkalpak Jun 21, 2016

Member

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

This comment has been minimized.

Copy link
@zbjornson

zbjornson Jun 21, 2016

Contributor

Aha okay, thanks for explaining :)

@petebacondarwin

This comment has been minimized.

Copy link
Member Author

petebacondarwin commented Jun 21, 2016

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

This comment has been minimized.

Copy link
Member

gkalpak commented Jun 21, 2016

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

@petebacondarwin petebacondarwin force-pushed the petebacondarwin:pr-14656 branch from de17903 to 2dcbc4e Jun 22, 2016

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

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.