feat($compile): add preAssignBindings flag

[petebacondarwin]

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

feature

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

Bindings are assigned to controller objects before their constructors are called

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

A flag can now be set so that bindings are assigned to controller objects after their constructors are called

Does this PR introduce a breaking change?

no

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:

A new flag to enable/disable whether directive controllers are assigned bindings before
calling the controller's constructor.

If enabled (true), the compiler assigns the value of each of the bindings to the
properties of the controller object before the constructor of this object is called.

If disabled (false), the compiler calls the constructor first before assigning bindings.

The default value is enabled (true) in Angular 1.5.x but will switch to false in Angular 1.6.x.

See #14580

[Narretz]

In line with a few other provider fns, I think we should call this one preAssign(ed)BindingsEnabled or something. (Enabled being the important part).

[mgol]

I can't comment on parseSpec.js changes inline as the diff is too big for GitHub so I'll write here: in the should cope with changes occuring inside '$onChanges() hooks test there is now nothing assigned in the controller. Also, most of the tests have moved the initial checks into the $onInit method. Am I missing something or is the constructor behavior for the flag set to false untested?

[petebacondarwin]

@Narretz - I agree. I will update the name of the flag getter-setter

@mgol - Github will show the diff if you ignore whitespare: https://github.com/angular/angular.js/pull/15095/files?w=s.

In the tests I took the approach of running a large collection of the tests with both preAssignBindings enabled and disabled. What I found was that there were a load of tests that checked for properties being assigned inside the controller. Obviously when our flag is set to false this doesn't happen and the correct place to check is in $onInit, which is the first point where we promise that bindings will have been assigned. So the approach I took was to run the check in the controller if preAssignBindings is true and inside the $onInit otherwise.

This covers almost all the changes in the specs, except for the one that you point out should cope with changes occurring inside '$onChanges() hooks. In this case I had previously added a "hack" to ensure that the property had a valid number in it at the start (i.e. this.prop1 = 0;) without this you get an extra call to $onChanges with the initial NaN value. This doesn't work if we are not preAssigningBindings. I wondered if this was actually a bug but I think that I was really just hiding the behaviour in the previous test.

[mgol]

@petebd I know about the w URL param but GitHub doesn't let one comment on
a diff like that so this doesn't buy me much. :)

Michał Gołębiowski

[petebacondarwin]

@mgol - true, true

[mgol]

@petebacondarwin I realize why you changed the logic to conditionally run the check in $onInit instead of a constructor. There are just no tests ensuring the flag actually does anything. If I change this line:

$compileProvider.preAssignBindings(preAssignBindings);

into:

$compileProvider.preAssignBindings(true);

all the tests still pass and they shouldn't.

[petebacondarwin]

Ah right! OK, I'll add an extra test that actually checks the difference

[petebacondarwin]

Fixed up via 4f599ff PTAL

[gkalpak]

This covers almost all the changes in the specs, except for the one that you point out: should cope with changes occurring inside '$onChanges() hooks. In this case I had previously added a "hack" to ensure that the property had a valid number in it at the start (i.e. this.prop1 = 0;) without this you get an extra call to $onChanges with the initial NaN value. This doesn't work if we are not preAssigningBindings. I wondered if this was actually a bug but I think that I was really just hiding the behaviour in the previous test.

I think it is actually a bug (depends on your POV). We fail to detect NaN === NaN (because it isn't 😁), so we are always reporting properties set to NaN as changed. Considering that (a) we have special NaN equality checks in a bunch of other places, (b) I can't think of any usecase where continously reporting NaN as changed is useful and (c) it is unintuitive, I think we should fix this.

Minor: Probably also the case for some other similar helpers, but there are no tests for the getter nature of $compileProvider.preAssignBindingsEnabled() 😁

Other than that LGTM 👍

[gkalpak]

We fail to detect NaN === NaN (because it isn't 😁), so we are always reporting properties set to NaN as changed.

Actually that is not exactly true. We are not reporting it constantly as changed, but we do report it one extra time (something that doesn't happen with non-NaN values).

In any case, it is not related to this PR. I would change the test to not be affected by this edge case.

[gkalpak]

I submitted a fix for this in #15098 😄