feat($rootScope): Add custom compare and copy functions to $watch

[Toilal]

Closes #10069

[Toilal]

Maybe if objectEquality is not a function, we could set angular.equals as the comparator function to avoid checking if comparator is undefined for each $digest cycle ?

Same for copier and angular.copy.

[mary-poppins]

I'm sorry, but I wasn't able to verify your Contributor License Agreement (CLA) signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let us know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

[Toilal]

CLA Should be OK now.

[Toilal]

It seems to work well for my use case (angular-gantt). I've created a custom watcher with equals and copier supporting momentJS properly through lodash. See https://github.com/angular-gantt/angular-gantt/blob/history/src/plugins/history.js#L24-L53.

[mary-poppins]

CLA signature verified! Thank you!

Someone from the team will now triage your PR and it will be processed based on the determined priority (doc updates and fixes with tests are prioritized over other changes).

[mary-poppins]

CLA signature verified! Thank you!

Someone from the team will now triage your PR and it will be processed based on the determined priority (doc updates and fixes with tests are prioritized over other changes).

[mary-poppins]

CLA signature verified! Thank you!

Someone from the team will now triage your PR and it will be processed based on the determined priority (doc updates and fixes with tests are prioritized over other changes).

[Toilal]

Is there anything to do to see this getting merged ?

[lgalfaso]

I cannot speak for the rest of the team, but there is something that make me unease about this PR. The digest cycle is a critical part of the code, and every single change has consequences. This is why there is a strict control to know when code from angular is executed and when code outside angular is executed, and be able to handle them with the right care. In this specific case, there are two points:

• we are 100% sure that equals and copy have no side-effects
• we can control that equals(instance, copy(instance)

If this PR is merged, then these assumptions are no longer true. I am specially worried about the first of these points

The PR also makes other small mistakes as it exposes the watch object as this to the copy and equals functions, but those are small nits that are easy to fix

[lgalfaso]

The more I think about this, I think that most of what this PR brings can be done within a third party module. It would be something like

/* Warning, untested code, and can use some cleanup */

angular.module('foo', []).service('watchHelper', function($parse) {
  return function(expression, customEquals, customCopy, listener) {
    var previousPreviousValue;
    var previousValue;
    var firstCall = true;
    var counter = 0;
    expression = $parse(expression);
    return {
      expression: function(scope, locals) {
        var newValue = expression(scope, locals);
        if (firstCall && !customEquals(newValue, previousValue)) {
          counter++;
          previousPreviousValue = previousValue;
          previousValue = customCopy(newValue);
        }
        return counter;
      },
      listener: function(newValue, oldValue) {
        if (newValue === oldValue) {
          listener(previousValue, previousValue)
        } else {
          listener(previousValue, previousPreviousValue)
        }
      }
    };
  };
});


// Then, in your code you do
var foo = watchHelper(expression, customEquals, customCopy, listener);
$scope.$watch(foo.expression, foo.listener);

[petebacondarwin]

@Narretz and I discussed a use for this functionality today inside ngModel.

I would like us to make a decision about whether we should include this:

• It seems to me that the normal use cases are untouched by this PR, which means that unless you decide to provide your own custom equals and copy the watching would be the same in terms of semantic and performance.
• While we would have no control over whether the custom equals and copy methods had side effects and were fast, this is not really any different than concerns over the user provided watcher functions.
• Although this can be done using a helper, as suggested by @lgalfaso, this would be less performant than if it was embedded in the core root scope, and is more fiddly to set up.
• I think we should ensure that a custom equals must be accompanied by a custom copy function.

[petebacondarwin]

The $watchDelegate also takes the objectEquality parameter, so this would need to be refactored too. Luckily this is an internal interface so we might be able to get away with it.

[Narretz]

Do we need an extra test for expressions that generate a watchDelegate?

[petebacondarwin]

Oh I forgot about this bit. There is a quite a bit of work to do here as each of the delegates in the parser need to be updated. We definitely need to test this.

[petebacondarwin]

TODO:

• Update watch delegates with this functionality too
• Check that both equals and copy are provided
• Add better docs
• Add bench press tests

[lgalfaso]

if we decide to move forward, then this looks like the wrong approach. We should not add new properties, and change get and listener so they are new functions that do the custom compare and copy.

#10096 (comment) can be used as a template, but in this specific case it can be simplified by using the $watch function closure.

If implemented that way, then there is no need to change any of the delegates