angular.js icon indicating copy to clipboard operation
angular.js copied to clipboard

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

Open Toilal opened this issue 11 years ago • 11 comments

Closes #10069

Toilal avatar Nov 17 '14 15:11 Toilal

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.

mary-poppins avatar Nov 17 '14 16:11 mary-poppins

CLA Should be OK now.

Toilal avatar Nov 17 '14 16:11 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.

Toilal avatar Nov 17 '14 18:11 Toilal

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 avatar Nov 19 '14 18:11 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 avatar Nov 19 '14 20:11 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 avatar Nov 19 '14 21:11 mary-poppins

Is there anything to do to see this getting merged ?

Toilal avatar Jan 07 '15 07:01 Toilal

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 avatar Jan 07 '15 11:01 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);

lgalfaso avatar Jan 09 '15 17:01 lgalfaso

@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 avatar Sep 30 '15 15:09 petebacondarwin

TODO:

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

petebacondarwin avatar Oct 01 '15 12:10 petebacondarwin