Properties set before constructor is called when using bindToController

[suedama1756]

Note: for support questions, please use one of these channels: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#question. This repository's issues are reserved for feature requests and bug reports.

• Do you want to request a feature or report a bug?

BUG

• What is the current behavior?

When using bindToController properties are being set before the constructor is invoked.

• If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via
  https://plnkr.co or similar (you can use this template as a starting point: http://plnkr.co/edit/tpl:yBpEi4).

bindToController: {
    min: '@',
}

class MyController {
  private _min : number; 
  constructor() {
     this._min = 0;
  }

  get min() {
    return this._min;
  }

  set min(value : number) {
     this._min = value;
  } 
}

• What is the expected behavior?

Constructor should be called before properties are set.

• What is the motivation / use case for changing the behavior?

Behaviour in properties setters that rely on values initialized in the constructor fail.

• Which version of Angular, and which browser and OS does this issue affect? Did this work in previous
  versions of Angular? Please also test with the latest stable and snapshot versions.

Windows 7, TypeScript 1.8.2, chrome 49.0.2623.87, Angular 1.5.0

• Other information (e.g. detailed explanation, stacktraces, related issues, suggestions how to fix)

[suedama1756]

The constructor is called in the following lines after the properties have been set in nodeLinkFn.

var controllerResult = controller();
if (controllerResult !== controller.instance) {

The properties setters created using Object.defineProperty are invoked even though the constructor has not yet been invoked. This is problematic if the property setters rely on values initialized in the constructor (in my case throwing exceptions).

[gkalpak]

This actually a feature not a bug :)

We are binding the properties, so you can work right away from the constructor with them.
(Note: This only works with regular functions not classes (but your TypeScript Class obviously gets transpiled to a plain old ES5 constructor function).)

Maybe you could check if _min is undefined before assigning a default value in the constructor.

[suedama1756]

With respect, I would suggest breaking the semantics of classes is a BUG and not a feature. As ECMA 6 becomes standard with property getters and setters this type of issue will become more common. It is called a "constructor" after all and controllers in all other areas of angular would have their constructors called before any other interaction (AFAIK).

I understand that it is difficult to understand how many people may be relying on this "feature" and that changing the behavior would probably have to be supported through an option for backward compatibility.

My issue was not with the constructor throwing an error, it was with the property setter assuming the state of the object was initialized and valid (the specific code is not shown in the example).

[dpogue]

Maybe you could check if _min is undefined before assigning a default value in the constructor.

I think that would fail if you had something like this:

class MyController {
  private _min : number = 0;

  constructor() { }
}

The resulting JS automatically sets _min in the constructor function:

var MyController = (function () {
    function MyController() {
        this._min = 0;
    }
    return MyController;
}());

[suedama1756]

I appreciate people are taking time out to provide workarounds; however, I'm not really looking for a work around; to be frank, its a trivial issue to resolve and I probably shouldn't be writing Javascript if I can't workaround it:).

I'm simply here to report the bug, which I still class as a bug. An APIs success is measured on its consistency. Every special case goes against this; an effects productivity, controllers in angular everywhere else are dealt with as types (with constructors), they are not functions.

[drpicox]

As docs says in: https://docs.angularjs.org/api/ng/service/$compile :

Deprecation warning: although bindings for non-ES6 class controllers are currently bound to this before the controller constructor is called, this use is now deprecated. Please place initialization code that relies upon bindings inside a $onInit method on the controller, instead

May be it should be changed ASAP. ¿Is there any scheduled change yet?

[suedama1756]

@drpicox Thanks for finding that; it sounds like the API is going to change to be consistent, as you say; the only question is when?

[dcherman]

While I agree with this idea in principle, you do also have to keep in mind that while the change is trivial, it's also potentially massively breaking for any historical code that uses assignments within the constructor; $onInit didn't exist until extremely recently.

That deprecation was introduced in 1.5, so I would expect that a breaking change of this magnitude would not be made until 1.6 or even 1.7 (Should really be in a major version, but Angular2 kinda killed that possibility it seems like without causing massive confusion).

I haven't though about this for more than 15 seconds (quite literally), but another interesting idea would be that if a controller lacks on $onInit method, then the constructor is invoked when $onInit would have been. Both occur at roughly the same time prior to prelinking, and I'm not sure whether or not there would even be any observable side effects from that. That could possibly allow this to be introduced in a non-breaking way.

[gkalpak]

if a controller lacks on $onInit method, then the constructor is invoked when $onInit would have been

I haven't thought about it for more than 15 seconds either (quite literally), but I think this would break the expectation that all controllers are initialized when $onInit is called.

[drpicox]

I get a thought to this "issue", and I think that may be there is a mid-way between break compatibility or do interfere in the use some ES5 features (like getters and setters).

There are some examples in angular, that because some reason a breaking change is introduced, but there is a possible configuration that enables old/new behaviour. For example:

• https://docs.angularjs.org/guide/production : $compileProvider.debugInfoEnabled(false); and {strictDi: true}
• ... (I cannot recall right now other examples)

So, it might be something like this would be an option:

$compileProvider.bindToControllerBeforeConstructor(false);

and change the default in later versions.

[DavidSouther]

As @dpogue points out, this behavior breaks a common pattern (default true and binding to false).

This "feature" irrevocably breaks the understanding of how constructors work.

I continue to fail to see how binding properties to an instance is a sensible operation when, by definition, the instance is in an undefined state before a constructor finishes executing. A reasonable flow would be [constructor(), bindings, $onInit].

[gkalpak]

Thanks for your feedback everyone. Here is a little more context (which will hopefully give better perspective of the situation):

A reasonable flow would be [constructor(), bindings, $onInit].

$onInit has only recently been introduced.

I continue to fail to see how binding properties to an instance is a sensible operation

It will make more sense if you stop thinking were we are and focus on how we got there and how to get out of there 😃

How did we get here?

1. First, there were linking functions and bindings were bound on the scope when these functions were called (so developers were used to taking them for granted).
2. Then, we got controllers on directives and developers started using them, mostly injecting the scope and again assuming that bindings were already bound to it.
3. Then, came the controller as/ditch-the-scope paradigm and people started switching to it.
4. Short after, it became necessary to be able to bind the properties on the controller itself, instead of the scope. At that point, where no lifecycle hooks existed and people assumed they had access to the bindings (previously on scope) from inside the controller constructor, the "feature" of pre-assigning them on the controller instance before calling the contructor was an actual feature.

How do we get out of here?
There was no doubt that this "feature" came at a cost (which is highlighted more by the rise of classes, since it is not compatible with them). Nobody wanted to be there, but we didn't have anywhere to go.
(Note that the feature has been marked as deprecated for quite some time.)

Things have changed and we do now have the means to get out (thanks to controller lifecycle hooks). Still we should get out of there without breaking all these apps that are currently (reluctantly or not) relying on this "feature".

So, here is the general plan (there are still some details to figure out):

1. We will provide a way to opt-out of this behavior in 1.5.
2. We will disable the "feature" by default in 1.6, but still provide a way to opt-in.
3. We will remove the "feature" in 1.7.

this behavior breaks a common pattern

tl;dr
One supported pattern is no compensation for a ton of unhappy users. But we have a plan... 😉

---

_{In defense of the "feature", I believe it has served us very well and we would have been in a worse situation without it - but it's time for it to go now 😃}

[DavidSouther]

@gkalpak Thank you so much for that write up! Having been with Angular since 0.9, I kind of decided to forget about a lot of that pain; and point #4 especially was a trap I personally never fell in to. Being reminded of it is humbling :)

[ashclarke]

This is not strictly related to ES6, but I also use ES5 Object property getter/setters that expose "private" values and have a similar issue.
For this use case, it would be good if the $onInit function was called with the bindings as a parameter, rather than having them set directly on the controller after they are parsed.

Here's an excerpt:

function ThingController() {
    var _name;

    function $onInit(bindings) {
        _name = bindings.name;
    }

    controllerProperties.name = {
        get: function get() {
           return _name;
        }
    };

    controllerProperties.otherThing = {
        get: function get() {
           return "Example of something else " + _name;
        }
    };
}

[gkalpak]

With #15095 merged, our plan has been materialized. It will be possible to opt-out of bindings pre-assignment from v1.5.9v1.5.10 on (not yet released) and "no pre-assignment" will be the default behavior in v1.6.0 (with the ability to opt-in).

[emillunde]

Was the option to opt-out included? There is no preAssignBindingsEnabled-function in $compileProvider in v.1.5.9.

[gkalpak]

@emillunde, we had to cut an interim 1.5.9 release that contains mostly security fixes. This feature will be included in 1.5.10 (which should be released pretty soon).

[ashclarke]

@gkalpak - Just to confirm, does this change introduced in 1.6 just delay the assignment of bindings to the controller instance, until just before $onInit is called?

Please let me know if you need me to clarify the question.

[gkalpak]

@ashclarke, exactly.

• In 1.5.10+, the default behavior will be to assign before calling the constructor, but you can change it to delay until before $onInit.

• In 1.6.0+ the default behavior will be to delay until before $onInit, but you can change it to assign before calling the constructor.

• In 1.7.0+ it will delay until before $onInit (and you won't be able to change that).