Native class syntax breaks invoking directive controller

[smalluban]

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

What is the current behavior?
Initializing controller created with native class syntax breaks directive compilation. Problem here is with pushing bindings from directive to self initalized controller instance before constructor is invoked, but ES2015 class syntax specification forbids using constructor without "new" operator.

Codepen example: http://codepen.io/smalluban/pen/eZBeNr?editors=1012

Console output:

console_runner-ba402f0….js:1 TypeError: Class constructor Controller cannot be invoked without 'new'
    at Object.invoke (angular.js:4604)
    at extend.instance (angular.js:9855)
    at nodeLinkFn (angular.js:8927)
    at compositeLinkFn (angular.js:8226)
    at compositeLinkFn (angular.js:8229)
    at publicLinkFn (angular.js:8106)
    at angular.js:1696
    at Scope.$eval (angular.js:16820)
    at Scope.$apply (angular.js:16920)
    at bootstrapApply (angular.js:1694)

What is the expected behavior?

Pushing bindings to class constructor can not be fixed. With ES5 ordinary function everything works fine, but using class syntax is not safe if you don't use transpiler(like Babel). Only way is to deprecate it and push values after class is constructed, but I have no idea if this is possible (due to fact of using this functionality).

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

There is a lot of articles where we are encourage to use class syntax for creating controllers. But using classes is danger, because application will break if you use native classes.

In my opinion using Babel to transpile class syntax is temporary. Most modern browsers already support classes, so using Angular 1 in hybrid application could be used without transpilation.

Which versions of Angular, and which browser / OS are affected by this issue? Did this work in previous versions of Angular? Please also test with the latest stable and snapshot (https://code.angularjs.org/snapshot/) versions.

Angular: 1.5.0
Browser: Every browser where class syntax is supported: Chrome 49+, Edge 13+, Firefox 45+, Safari 9+.

[gkalpak]

It works fine for me on Chrome 49 (Win 10).
The injector (which is used for instantiating the controllers) is supposed to support native Classes (but it's not possible to "pre-assign" the bindings on this).

What browser/OS did you test this on ?

[smalluban]

Tested with Macbook Pro / OS X El Capitan 10.11.3 (15D21):

Firefox 45 throws "Error: class constructors must be invoked with |new|"
Chrome 50 (I am using beta channel) throws "TypeError: Class constructor Controller cannot be invoked without 'new'"
Safari 9.0.3 throws "Error: Cannot call a class constructor"

The injector has "isClass" method which returns false for native class. But, Do bindings should work differently using "real" classes opposite to transformed to ES5? If my application use this behavior to initialize controller properties, when I switch to native classes, it can stop working properly.

[gkalpak]

OK, so this error occurs when we fail to detect that a constructor is a Class rather than a function.
This usually happens due to bugs/deviaton from spec in browser implementations of native classes.

More specifically:

• Firefox 45:
  We are affected by this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1216630
  I.e. the string representation of class MyClass {} is function MyClass() {}, which violates the 3rd requirement for toSting() as described here
• Chrome 50/51:
  There seems to be a bug where the string representation of class MyClass {} can be either class MyClass {} (which is correct) or function class MyClass {} (which is incorrect and break the $injector).
  "Normally" the latter (incorrect) version is used by Chrome, but if you set a breakpoint and step into (or over) the $injector.invoke(...) call, then the former (correct) version is used and everything works as expected !?
• Safari 9:
  Same as with Firefox 45: class MyClass {} --> function MyClass() {}.

[smalluban]

@gkalpak What do you think about my question? Should there be a different behavior for inserting bindings to native implementation of class and ordinary function constructor? Consider this code:

class Contr {
  constructor() {
    // expression is a '&' binding
    this.data = this.expression();
  }
}
app.component(..., { controller: Contr } );

When application uses Babel, controller will be a function (ES5) and bindings will be present in constructor (injector will push them). Application works fine. But, when native class would be used, injector will know that and will not push bindings. Application breaks with TypeError - this.expression is not a function...

In my opinion, it can be confusing when application starts to fall if I turn off transforming from ES6 to ES5.

[gkalpak]

Removing the current behavior would be a big breaking change and we don't want that. Yet, accessing the properties inside the constructor is already deprecated and the recommended practice is using $onInit().
This is already documented:

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.

I don't think there is much more we can do, other than doing our best to properly detect native classes.

[uriva]

This is currently breaking our production app. Any updates or a temporary workaround?

[thorn0]

isClass also doesn't work for me in this plunker. I'm getting this in two versions of Chrome: 1) Chrome 50.0.2661.87 m, Windows 7 x64; 2) Chrome 50.0.2661.86 (64-bit), OS X 10.9.2.

This bug isn't stably reproduced. If I edit something seemingly unrelated in app.js, the bug may disappear (e.g. if I remove the definition of the myCompo component). I tried to debug it by using a modified version of angular.js to save the stringified function/class and the regex (/^(?:class\s|constructor\()/) used for class detection to global variables. Then I played with those saved values in the console and found out that regex.test(str) returns false. The class is stringified normally, like class {.... However, this string contains \r characters. When I removed them (str=str.replace(/\r/g,'')), the regex for some reason started to work. Also the regex works if I recreate it manually. It's actually the strangest part: two identical regexen, the saved one and the one created manually, give different results.

BTW, I also found that (class{}).toString() evaluates to 'class{}', which isn't caught by the regex.

[gkalpak]

This sounds similar to #14487. @thorn0, can you verify if the problem is stil reproducible on v52 ?

[thorn0]

I can't reproduce it in 52.0.2715.0 canary (64-bit), Windows.

[gkalpak]

#14531 should take care of it (until Chrome v52 is released).

[jgrund]

Should this be closed? I'm seeing native classes failing in Firefox 48:

class constructors must be invoked with |new|

[gkalpak]

@jgrund, it is a bug in Firefox (see #14240 (comment)). We can't really do much 😞

[graingert]

I guess you can create classes with: (with modern-browsers, and stage-1 transpilers)

class Foo {
    static $inject = ['$window']
    static toString() {
        return 'class Foo {}';
    }
    constructor($window) {
        this.$window = $window;
    }
}

[gkalpak]

I don't think it will work, because we are calling Function.prototype.toString.

[lgalfaso]

It will depend on how

static $inject = ['$window']

Is propagated out

On Friday, July 1, 2016, Thomas Grainger [email protected] wrote:

I guess you can create classes with: (with modern-browsers, and stage-1
transpilers)

class Foo {
static $inject = ['$window']
static toString() {
return 'class Foo {}';
}
constructor($window) {
this.$window = $window;
}
}

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#14240 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAX44ovdB_et65iyUnrHTCbigUWPYjNgks5qRNpzgaJpZM4HxAWJ
.

[graingert]

class Foo {
    static $inject = ['$window']
    static $$isClass = true
    constructor($window) {
        this.$window = $window;
    }
}
(function () {
    oldToString = Function.prototype.toString;
    Function.prototype.toString = function toString() {
        const defaultString = this::oldToString();
        if (!this.$$isClass) {
            return defaultString;
        }
        return defaultString.replace(/^function ([^(]+)\(([^)]*)\)([\S\s]*)/, 'class $1 {constructor($2)$3}');
    }
})();
Function.prototype.toString.apply(Foo);

"class Foo {constructor($window) {
        this.$window = $window;
    }}"

[graingert]

(function Ham(spam, eggs) {this.foo = spam}).toString().replace(/^function ([^(]+)(([^)]))([\S\s])/, 'class $1 {constructor($2)$3}')
'class Ham {constructor(spam, eggs) {this.foo = spam}}'

[graingert]

lgalfaso it's transpiled to:

class Foo {...}
Foo.$inject = ['$window']

[gkalpak]

@graingert, it is probably not a good idea t patch Function.prototype.toString like this, but it is also not necessary if we assume access to $$isClass. If $$isClass is set, then we don't try to stringify the function/class.

But the problem with $$isClass is that it will only work on 1.6 and is a private property, so might break without notice.

[graingert]

I just made up $$isClass you can replace it with whatever. I just created a
workaround people can paste into their projects.
On 1 Jul 2016 12:39, "Georgios Kalpakas" [email protected] wrote:

@graingert https://github.com/graingert, it is probably not a good idea
t patch Function.prototype.toString like this, but it is also not
necessary if we assume access to $$isClass. If $$isClass is set, then we
don't try to stringify the function/class.

But the problem with $$isClass is that it will only work on 1.6 and is a
private property, so might break without notice.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#14240 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAZQTJDMjSh89F1BlcTzwvZwSnW-X4Cuks5qRPxogaJpZM4HxAWJ
.

[gkalpak]

Right! I was confused, because there is actually a similar property that we attach to functions/classes, but it is called $$ngIsClass 😃

So, fair enough! It is a viable work-around until browsers catch up with the spec (but I never officially recommended it 😛)

[hashseed]

In the few minutes of playing around, I haven't been able to reproduce "function class MyClass" on Chrome. Can you give me some pointers?

class MyClass {}
console.log(MyClass.toString()) // This logs "class MyClass {}"

[gkalpak]

@hashseed , iirc it doesn't happen if you console.log from the console. But still when I write class MyClass {} in the console and hit enter, it displays function class MyClass {} (despite .toString() working as expected).
I assume it related to some internal optimization, so it only happens inside a function call. But it is fixed on v52.

[hashseed]

@gkalpak that I can indeed reproduce. However, given the bug description, I was hoping for a way to get the "function class" string from within the script. Do you have an example for that?

[gkalpak]

@hashseed, theoretically, you should be able to reproduce it with OP's demo. But it isn't reproducible on my Chrome v51.0.2704.106 any more (probably fixed).

[graingert]

is there anyway of opting into the deprecation and preventing those bindings from being accessed?

[gkalpak]

@graingert, not yet (but the plan is to provide a way to opt-out). (See here for more context.)

[katrotz]

@gkalpak The codepen example mentioned in the bug description still failing in FF 50.1.0

[graingert]

Looks like this is fixed in 1.6

[gkalpak]

@katrotz: It fails because Firefox does not stringify classes in a way that allows Angular to recognize them. A hacky work-around (in the sense that it is private and might break any time) is to put a static property on the class $$ngIsClass and set it to true:

class Foo {}
Foo.$$ngIsClass = true;

[TitanNano]

wouldn't it be possible to try...catch the controller construction and retry with a new statement if the error message was thrown?

[gkalpak]

@TitanNano, we have decided against this, since a constructor might have side effects (so you don't want to call it twice) and there is no reliable way (or at least there was none at the time) to identify the cause of the error (e.g. each browser threw a different error etc).

[johannesjo]

Still an issue for me with 1.6.1.

[gkalpak]

@johannesjo: The comments above explain why it happens and how to work around. If you are running into a new issue that is not discussed above, please open anew issue (with more details).

[johannesjo]

@gkalpak I hope that doesn't sound rude, but a 'hacky workaround' is not a real fix, wouldn't you agree? A client library should deal with browser quirks even when the browser is 'wrong'.

[graingert]

@johannesjo seems like it's impossible to fix without try...catch

[johannesjo]

@graingert I see the issue, but to me it sounds like it would be possible to fix this, but only in a very nasty unfun way (handling lots of different browser specific errors). Depending on how reliable the error handling could be (well it should be at least 99.8% percent ;)) I stil think that it might be better than to live with a possibly unreliable workaround.

[galadhremmin]

The $$ngIsClass hack, while not pretty, solved the issue for me. While it's an abuse of internals, Mozilla will hopefully fix this bug before $$ngIsClass is deprecated.

I also believe a try...catch solution could potentially catch exceptions thrown during object construction, and thus render false positives.