$controller cannot instantiate ES6 classes

[trevorade]

ES6 classes can only be instantiated using new. $controller uses $injector to instantiate the classes which even after this fix: #12598 uses fn.apply.

In particular, this should work when bindToController is used.

[mprobst]

See comment on linked PR:

#12614 is not fixed by this – $controller uses $injector, but bindToController has its own logic to instantiate controllers on directives. I'm not sure it'll be possible to fix bindToController, it's way of instantiating a class on top of an existing object, then applying the ctor to it might be inherently incompatible with ES6.

[bathos]

@mprobst It's not just an ES6 issue. The bindToController implementation is also incompatible with ES5, ES3 ... JavaScript, period, because no action is taken to carry the prototype of the original constructor over to the artificial target. Turning on "bindToController" will break any controller in any version of JS if it makes use of its prototype. I haven't dug deep into the code there yet, but it seems like this could be (more or less) fixed simply by having the artificial target begin as Object.create(RealConstructor.prototype).

Also, the behavior they want to achieve can be implemented like this (in its desugared ES5 version):

const BindWrapper = class extends ActualController {};
// now do all the scope stuff to this one-off BindWrapper's prototype; those properties will be available
// then when the constructor is instantiated (with new!) without having to modify ActualController

[gkalpak]

Turning on "bindToController" will break any controller in any version of JS if it makes use of its prototype.

@bathos , ooc could you provide an example. It has worked for me in the past for simple cases (I think), but I haven't extensively used prototype chains in directive controllers tbh.

[gkalpak]

it seems like this could be (more or less) fixed simply by having the artificial target begin as Object.create(RealConstructor.prototype)

Isn't that exactly what's happening in controller.js#L128-L130 ?

[bathos]

@gkalpak Yes, that does look sound. But is that actually what's happening when bindToController is in play? @mprobst's comment made it seem that that was not the case, and in my case, controllers are only affected if bindToController is used. That said, I also haven't tried it with generalized test cases and couldn't say for sure yet that there aren't local factors in the results I've seen.

[gkalpak]

I think @mprobst's comment was about ES6 classes, not "plain" constructor functions.

couldn't say for sure yet that there aren't local factors in the results I've seen

I would be still curious to take a look at those examples, if you can post them in CodePen, Plnkr etc.

[bathos]

@gkalpak Sorry not to follow up sooner, but also because I was rather emphatic about something which wasn't really true. (It really looked like it was, though!) The problem I was encountering stemmed from the fact that in my own code, I had a class decorator that essentially did the same thing, but made incorrect assumptions about how Angular would consume the class. It was an injection decorator that aggregated injections on $inject and then generated a one-off class per instantiation whose prototype it augmented with the injections as properties (for easy in-method access and no overlong constructor signature) before returning that new instance. After tweaking its implementation, everything did fall into place, and I shouldn't have expected Angular to like the way I was doing it initially.

[gkalpak]

@bathos, np 😃 I'm glad that everything fell into place after all !

Let's see if we can find a way to support ES6 classes (if at all possible)...

[bathos]

@gkalpak, what part of it isn't working now? I am successfully using es6 classes with bindToController in 1.4 now that I've found my own error. It seems to be instantiating them correctly.

[gkalpak]

Is that so ? That's unexpected (by me at least). Are you using native classes (i.e. no transpiling to ES5 or something) ?

[bathos]

Non-native, but with Babel 5, whose transpiled classes include an instantiation vs invocation check.

That non-native check could be worked around (e.g. const bindTarget = Object.create(Ctrl.prototype); /* augment with bindings here */ bindTarget.constructor();) because all one would need to do to fool it is call it as a method (or with call) on an object which can pass the instanceof check (and using Object.create sets us up correctly for that). I imagine this approach would fail with native classes.

However, I don't think that's what's being done. It looks like controllers with bindToController do eventually pass through $inject.instantiate (now, anyway) which does this:

return new (Function.prototype.bind.apply(ctor, args));

The clever use of applying bind there is needed since the length of args is unknown; nonetheless, this is a genuine instantiation. Am I missing something where the btc controller gets in trouble along the way?

[gkalpak]

@bathos, the bindToController feature requires some special logic and does not rely on $injector.instantiate(), but $injector.invoke() (see #12598 (comment)).

So, atm bindToController should not be compatible with ES6 classes (well it should, but it isn't).

[gkalpak]

To recap:
$controller() has two different code paths (decided upon based on the later argument).
One using $injector.instantiate() and the other using $injector.invoke() (among other things).

In order for $controller() to support ES6 classes, both paths need to support them.
#12614 takes care of making $injector.instantiate() class-ready.
#13510 tracks the progress on making the other path compatible with ES6 classes.

[bathos]

Ah, I missed the other pathway. Makes sense: the invoke logic uses fn.apply(self, args), where self is Object.create(fn.prototype), which explains why it does work with transpiled classes (which can be fooled this way) but not native classes. Thanks for all the info, it's good to know more about angular internals.

[gscoppino]

Tested using $controller with a transpiled ES6 class (via babel) on the 1.5.0 stable release, works as expected.

class AppController {
    constructor($rootScope) {
        this._$rootScope = $rootScope;
        this._$rootScope.test = 42;
    }
}
AppController.$inject = ['$rootScope'];

const app = angular
    .module('app', [])
        .controller('AppController', AppController)
        .run(function () {
            console.log('Loaded!');
        });

beforeEach(module('app'));
describe('app', function () {
    describe('app.controller', function () {
        it('should set test property on $rootScope.', inject(function ($injector) {
            let $rootScope = $injector.get('$rootScope');
            let $controller = $injector.get('$controller');

            $controller('AppController');
            expect($rootScope.test).toEqual(42);
        }));
    });
});

[gkalpak]

This has indeed been fixed by #12598 and #13682 (and followup PRs).
Thx @gscoppino for verifying :)