fix($compile): don't run unnecessary update to one-way bindings

[petebacondarwin]

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

Bug Fix

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

The watch handler was triggering on its first invocation, even though
its change had already been dealt with when setting up the binding.

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

The handler does not trigger an update on its first invocation

Does this PR introduce a breaking change?

Nope - I don't think so. Although I had to modify a bunch of tests to ensure that a digest occurred between compilation and the first checks.

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:

Closes #14546

[gkalpak]

Why is this needed ?

[petebacondarwin]

If we have no digest before changing the value of val then effectively the system thinks that val has not changed, so $onChanges is not called.

[gkalpak]

You are pushing to log, but never checking what was actually pushed.

[petebacondarwin]

Fixed.

[gkalpak]

One testcase I believe should work, but doesn't, is when the parent value changes after initializing the bindings but before the first call to the one-way binding watcher callback.
Here are a couple of unit tests demonstrating the issue. They are essentially equivalent, the first being more straight-forward and the latter being more "real-world-ish".

      it('should update isolate again after $onInit if outer has changed (before initial watchAction call)', function() {
        var log = [];
        var component;
        angular.module('owComponentTest', [])
          .component('owComponent', {
            bindings: { input: '<' },
            controller: function() {
              component = this;
              this.input = 'constructor';
              this.$onInit = function() {
                this.input = '$onInit';
              };
              this.$onChanges = function(changes) {
                if (changes.input) {
                  log.push(changes.input);
                }
              };
            }
          });
        module('owComponentTest');
        inject(function() {
          $rootScope.name = 'outer1';
          compile('<ow-component input="name"></ow-component>');

          expect(component.input).toEqual('$onInit');
          $rootScope.$apply('name = "outer2"');

          expect($rootScope.name).toEqual('outer2');
          expect(component.input).toEqual('outer2');
        });
      });

      it('should update isolate again after $onInit if outer has changed (before initial watchAction call)', function() {
        var log = [];
        var component;
        angular.module('owComponentTest', [])
          .directive('changeInput', function() {
            return function(scope, elem, attrs) {
              scope.name = 'outer2';
            };
          })
          .component('owComponent', {
            bindings: { input: '<' },
            controller: function() {
              component = this;
              this.input = 'constructor';
              this.$onInit = function() {
                this.input = '$onInit';
              };
              this.$onChanges = function(changes) {
                if (changes.input) {
                  log.push(changes.input);
                }
              };
            }
          });
        module('owComponentTest');
        inject(function() {
          $rootScope.name = 'outer1';
          compile('<ow-component input="name" change-input></ow-component>');

          expect(component.input).toEqual('$onInit');
          $rootScope.$digest();

          expect($rootScope.name).toEqual('outer2');
          expect(component.input).toEqual('outer2');
        });
      });

[gkalpak]

BTW, in order to support these usecases (and fix the original bug), we could change the code from:

destination[scopeName] = parentGet(scope);
...

removeWatch = scope.$watch(parentGet, function parentValueWatchAction(newValue, oldValue) {
  if (oldValue === newValue) return;
  ...

to:

var initialValue = destination[scopeName] = parentGet(scope);
...

removeWatch = scope.$watch(parentGet, function parentValueWatchAction(newValue, oldValue) {
  if (oldValue === newValue) {
    if (oldValue === initialValue) return;
    oldValue = initialValue;
  }
  ...

[gkalpak]

Wouldn't this test pass even without the fix ? (I haven't tested it, but I think so.)
I think it's a good idea to add a $rootScope.$digest() and another round of expectation to make sure that nothing is overwritten when the watch callback is executed.

[petebacondarwin]

OK. It did fail before the fix. But then I might have changed it again since then.

[petebacondarwin]

It totally fails as it is without the fix. But I will add the extra digest and check

[gkalpak]

A couple of minor test-related comments.
Otherwise LGTM 👍

[ghost]

Hey, this just broke a whole bunch of our unit tests which I was hoping to get some clarification on.

So right now, we have something like this:

export someComponent = {
  ...

  controller: class SomeController {
    constructor(...) {
      this.state = { ... }
    }

    set config(config: Config) {
      if (!this.state) return
      ...
      this.state.config = config
    }
  }
}

angular.component('someComponent', someComponent)

The reason for the if (!this.state) return line is because prototype accessors are called before the constructor, and we need to do a number of things in the constructor (not least setting up this.state) before the config setter is called again.

We can't move the constructor code since we rely on injected dependencies to do some setup. Perhaps there's a better pattern for what we want to do but not sure how our use case can be accomplished. Do you have any ideas on what to do here?

[gkalpak]

TBH, I don't get what the problem is. Could you share some more code and/or tests.

[ghost]

Hey @gkalpak the issue we're encountering is basically very similar to #13590 - I guess we just need to work around it.

[ghost]

Just to give you some context, setters got called twice on startup (once before the component controller instantiates and once after). We relied on the behavior that setters got called after instantiation since dependencies are available at that point. Since this change returns early for matching values, our setter doesn't execute.

[ghost]

OK I see the angular way of doing this is to hook into $onChanges 😃 - thx!

[gkalpak]

Glad you got it working with $onChanges 😃
I'm still not sure what your code does and how it is afected by this, but you might also want to look into $onInit.

[bcherny]

@gkalpak I think what @khoomeister is saying is that in 1.5.5, the order of calls was setter-constructor-setter. So on the first fire this.state was undefined and the setter returned early, then the constructor defined this.state, then the setter fired again (successfully this time, because this.state was now defined).

In 1.5.6, that 2nd call to the setter never happens, so the setter is never fully executed.

This is a breaking change for anyone that relies on something being defined on this in a setter!

[gkalpak]

@bcherny, it will be easier to investigate with an actual example, but this is expected behavior as far as I understand. The fact that it worked differently in 1.5.5 (i.e. overwriting the inner value even though the outer vaue hadn't changed) was a bug. This commit is restoring the expacted/correct behavior and thus is not considered a breaking change (although it would break a usecase that relied on the buggy behavior).

The proper way to ensure something is run after the bindings have been initialized is using an appropriate hook (e.g. $onInit or $onChanges - depending on the usecase).

[bcherny]

@gkalpak Demo here - http://codepen.io/anon/pen/EyVyzB

[gkalpak]

@bcherny, fwiw it is expected behavior. The actual "problem" with your usecase is not how one-way binding watches are triggered, but the fact that we are pre-assigning the binding values on the controller instance before actually instantiating it.

Although this is supposed to be a feature, this has also created its fair share of headaches as well, so it is scheduled for deprecation/removal in future versions. (We might even provide a way to opt-out in 1.5.x as well.)
Note, that the behavior will be different, depending on whether or not your controller is recognized as a class (vs plain old constructor function).

[bcherny]

A backwards compatible way to make it work (at least in my case) would be to get rid of the 1st binding fire (caused by pre-assigning) rather than the 2nd binding fire -- is that what you mean?

So in 1.5.5 it was:

call setter - call constructor - call setter

In 1.5.6 it's:

call setter - call constructor

And what I'm proposing is:

call constructor - call setter

[petebacondarwin]

call constructor - call setter is what we are aiming for in 1.6 but this would certainly break even more apps than the current change in 1.5.6

[bcherny]

Good to hear - at least for me, that would be an improvement!

[gkalpak]

Note that this is the behavior you might get today if your controller is a class (a real class).
I think it would be nice to introduce the new behavior as an opt-in in 1.5.x, so people that are eager for this get a chance to try it out early and we gather some feedback before 1.6 is out.
Something like:

$compileProvider.enableBindingsPreAssignment(false);

The feature would be enabled by default on 1.5.x (so backwards compatible), but can be turned off.
Then on 1.6.x we will have it disabled by default (optionally allowing developers to turn it on).

[petebacondarwin]

@gkalpak I agree. But I think it should be simply:

$compileProvider.preAssignBindings(false);

[RobbertWolfs]

@gkalpak @petebacondarwin Is there any progress on this?
Because we just upgraded from 1.5.5 to 1.5.8 and we also have binding issues..

We have a component that requires a boolean value but it is always set to the default value.. not the one we use in the binding..

Some code:

Se we pass true as a binding and if we don't have the binding the default value should be false. But now it is always set to false..

in our test:

it('it has a required binding', () => {
   $scope.required = true';
   element = createDirective('<image-upload required="required"></image-upload>', $scope);
   controller = element.controller('imageUpload'); 
   expect(controller.required).to.eql(true);
});

in our component

class ImageUploadController {
   // @ngInject
   constructor() {
      // DEFAULT BINDING
      this.required = false;
    }
}

export default {
   template: 'my template',
   controller: ImageUploadController,
   bindings: {
      required: '<?',
   },
};

[gkalpak]

@RobbertWolfs, in case you haven't noticed, the related PR has been merged and the preAssignBindingsEnabled configuration switch will be available in an upcoming 1.5.x release 🎉