fix($controller): support instantiation of ES6 classes

[petebacondarwin]

See #12598 (comment)

The $controller service has two paths for creating instances of a controller. In the later == true path we are not using $injector.instantiate, which has recently been fixed to support ES6 classes.

We ought to refactor $controller to use $injector.instantiate in both paths for consistency, if this is possible.

[lgalfaso]

Raw facts:

• A class constructors must be called using new (and this behavior cannot be replaced with Function.apply as there would be no new.target defined) https://people.mozilla.org/~jorendorff/es6-draft.html#sec-ecmascript-function-objects-construct-argumentslist-newtarget
• There is a currently defined behavior of bindToController that the values to be bound to the controller, are present during the controller initialization https://docs.angularjs.org/api/ng/service/$compile#-bindtocontroller-

These two things, AFAICT, are incompatible, and a breaking change will be needed to add support for ES6 classes and bindToController.

Proposed solution:

1. Change the behavior of bindToController and remove the mandate that the values must be present during the controller initialization. This is be a breaking change.
2. Recommend developers to use setters to detect when values are bound

An alternative to 2 is
3. When using bindToController, if the class contains a method called [[TBD]], then, after the initial values are set, this function is called

I would like to avoid 3 as it creates some complexities

[gkalpak]

TBH, I was in favor of something like (3).

The problems I see with (1) and (2):

a. It will be a major BC and one that will cause silent failures i most cases, so it will be even more difficult for people to migrate.

b. Using setters adds quite some amount of boilerplate.

c. Even with setters, some (not uncommon) scenarios might be difficult or inefficient to implement. E.g. quite often, I have used the scope bindings during controller initialization for one-off configuration (e.g. if this.someProp equals X, then operate in Y mode etc). This won't be so easy to do using setters, especially when multiple properties need to be taken into account together. You'd have to keep track of what properties have been set (until all properties you are intrested in are set), you should defer all dependent operations etc.
It's doable, but it won't be pleasant :)

What I think might be a good compromise is:

a. Having a special method, that will be called when the bindings are initialized. That way we can instantiate the object (using a constructor function or class), assign the properties to it and call it's special method.

b. TBD: I'm not sure if this is (reliably) possible or if it will be more helpful than confusing, but if we can deferrentiate between function and classes, I would consider keeping the current behavior for constructor functions and implement (a) for classes.
That would avoid the BC, but might catch people migrating from functions to classes by surprise.

@lgalfaso, what are the complexities you have in mind ?

[lgalfaso]

@gkalpak the complexities are not technical nor in Angular, but mostly on consistency that create complexities for developers. Please check my comment again

1. Change the behavior of bindToController and remove the mandate that the values must be present during the controller initialization. This is be a breaking change.

I want to remove the fact that it is always there, and for bindings to be there when using functions and not there when using classes (not transpiled)

This is, the initialization will need to be changed to

1. Initialize all scope bindings
2. Create instances of the prototype of the controllers without calling the constructor
3. Bind the prototype instances
4. Create all controllers with the new(Function.bind.apply(...)) trick
5. If the instance change, unbind and rebind

Adding a call me when ready would be adding
6. Call the bindingsReady method

I am not sure how this would be a breaking change that actually affects developers, they currently cannot use classes natively as these are not supported, but if at any point in time they do want to move to native classes, then they will need to do some changes. Until that happens, no change is required. This is where the recommendation is important. Developers expect that there is a good practice defined on when it is the controllers time to do their work. If the recommendation is that they have to do so on bindingsReady, then that would imply that simple functions are second class citizens (even when no browser but Chrome supports classes). Recommending the use of setters is a solution that is forward compatible, this is, when browsers start supporting classes, they do not need to modify their code. Recommending bindingsReady sounds like saying: "migrate to using classes", that I think is not right.

[thorn0]

@lgalfaso So you decided not to do this?

This is, the initialization will need to be changed to

1. Initialize all scope bindings
2. Create instances of the prototype of the controllers without calling the constructor
3. Bind the prototype instances
4. Create all controllers with the new(Function.bind.apply(...)) trick
5. If the instance change, unbind and rebind

[lgalfaso]

@thorn0 we already have all that infrastructure in place, the only change is in point 4. and that is what the PR does

[thorn0]

@lgalfaso Oh, I see. I thought you had some idea on how to make the bindings' values still available in the ES6 class constructor by means of substituting its prototype with a different object.

[thorn0]

This works:

class Controller { 
  constructor(x) {
    console.log(this.bindingValue);
    this.x = x;
  }
}

class DisposableSubclass extends Controller {};
var self = DisposableSubclass.prototype;

// add bindings
self.bindingValue = 100;

// this.bindingValue is available in the Controller's constructor now
var instance = new (Function.bind.apply(DisposableSubclass, [null, 2]));

[bathos]

I'm curious if the technique we've been use for decorating controllers & services with injections (so they appear on the instances, a lot like bindToController) might be applicable? The gist is that each instance is a born from a one-off child class, which is very much like "Create instances of the prototype of the controllers without calling the constructor" except that you don't lose the ability to instantiate normally. Gist:

class BoundCtrl extends Ctrl {
  constructor(bindings) {
    Object.assign(BoundCtrl.prototype, bindings);
    super();
  }
};
new BoundCtrl(bindings);

Depending on the particular needs, the child prototype augmentation could be done before invocation, too. Essentially it buys you a fake 'this' that can be touched before super, though. Not sure if this really represents a meaningful difference from the solution currently in play, but figured it was worth sharing.

Edit: @thorn0 weird that we were writing almost the same post at the same moment!

[lgalfaso]

@thorn0 it is not possible to write #13510 (comment) without an eval while keeping browsers that do not support class happy

[thorn0]

@lgalfaso Is eval totally prohibited in Angular's code? Would a one-time call to new Function really hurt?

@bathos Not having the bindings' values in the constructor, that's what would be weird.

[lgalfaso]

@thorn0 eval is blocked when csp is enabled. This is the reason why there are two implementations of $parse

[bathos]

@thorn0 the bindings would already be available in the constructor, that's the point of performing the child-prototype augmentation prior to calling its super constructor (the original parent). Unless I'm missing what you mean?

[thorn0]

@bathos I just meant that it's no wonder several people wrote about the same thing since not having the bindings in the controller doesn't sound good. However, it seems nothing can be done here.

[petebacondarwin]

Currently we don't see a way to allow the constructor function of a native ES6 class (i.e. not a transpiled class but a real class in a browser that supports it) to run against a previously defined instance.

What we will be able to guarantee is that for real classes you will not get bindings during the constructor but they will be there by the time of the next digest.

[bisubus]

@petebacondarwin It is the same for transpiled classes as well, Babel puts safeguards against invoking apply on unbound class constructor.