fix($injector): Add support for native Safari classes

[lgalfaso]

Change the mechanism to detect if a function is a class so it is compatible with Safari 9.

[petebacondarwin]

Interesting idea!

[lgalfaso]

@petebacondarwin I know this is not a perfect solution, but I am not sure what other workaround is possible

[gkalpak]

Sorry if it's obvious, but what is the purpose of () at the end.

[lgalfaso]

@gkalpak it is is the same difference as between var a = new Foo; and var a = new Foo();

[petebacondarwin]

@lgalfaso I think that @gkalpak is saying that the line that you removed does not have these extra parentheses...

[gkalpak]

OK, so it's obviously just to avoid the jsHint W058 error, right ? I thought it might have anythng to do with supporting classes in Safari or something (but I guess not).

[lgalfaso]

Yep, just to fix the warning

On Tuesday, January 19, 2016, Georgios Kalpakas notifications@github.com
wrote:

In src/auto/injector.js
#13785 (comment):

     return fn.apply(self, args);

•  } else {
  

•    args.unshift(null);
  

•    /*jshint -W058 */ // Applying a constructor without immediate parentheses is the point here.
  

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

•    /*jshint +W058 */
  

•  } catch (e) {
  

•    // if `fn` is a class, then it needs to be called using `new`
  

•    return new (Function.prototype.bind.apply(fn, [null].concat(args)))();
  

OK, so it's obviously just to avoid the jsHint W058 error, right ? I
thought it might have anythng to do with supporting classes in Safari or
something (but I guess not).

—
Reply to this email directly or view it on GitHub
https://github.com/angular/angular.js/pull/13785/files#r50149001.

[petebacondarwin]

It looks (from the JS specs) that it makes no difference in practice:

new Foo is equivalent to new Foo()

I wonder why @mprobst wanted to leave that out in the first place?

[mprobst]

I think that was not intentional, it just sort of made more sense to me combined with the bind. But this is fine.

[mprobst]

Isn't this quite risky? We're essentially invoking the same code twice, regardless of why this threw an exception. E.g. if the code that's called is killAllHumansOnSecondCall (or more realistically, submitShoppingCartBuyThenThrowException), wouldn't we accidentally buy two shopping carts?

I think this should at least inspect the thrown exception to make sure class vs no class was the cause.

Also note that try/catch brings you onto the non-optimized path in every VM - I'd suspect that the previous solution was better?

[petebacondarwin]

We could optimize the try-catch only for Safari 9 but I agree that there is a concern about running the constructor twice...

[petebacondarwin]

I think I agree with @mprobst that this approach could cause problems.
I don't see any other solution at this stage.
I am also aware that these buggy browsers will probably soon get their act together and fix themselves.

While we look for a better solution, we could document it as a "known issue” with Safari - i.e. that you can't use native ES6 classes if your users are likely to run Safari 9.0.x.

In any case I suspect that the number of real applications doing this right now are going to be very small (and probably fixed on a single browser manufacturer) until the ES6 classes are much more widely available.

[lgalfaso]

I somehow agree with @mprobst. Updated the PR so the constructor would only be called when the error thrown is a TypeError. This reduces the possible cases that users code would get called multiple times, but does not remove it completely

[gkalpak]

I wonder if we could get "even smarter" and try to ensure the error message contains some keyword(s).

Or if Safari is the only supported browser that "tricks" the isClass() function, then why not use it anyway and try to catch the specific error thrown by Safari ?

Just a few ideas off the top of my head (haven't tried anything)...

[petebacondarwin]

Safari 9: TypeError: Cannot call a class constructor
Chrome 47: TypeError: Class constructors cannot be invoked without 'new'
Babel generated: TypeError: Cannot call a class as a function

(It seems that the spec is ambiguous as to what message should appear in the error: http://www.ecma-international.org/ecma-262/6.0/#sec-ecmascript-function-objects-call-thisargument-argumentslist)

[lgalfaso]

Support for class will be in FF 45, but do not know what the error message will be

[dpogue]

Firefox's error message is TypeError: class constructors must be invoked with |new|

[mgol]

I wouldn't rely on error messages when they're not standardized and every browser do them in a different way. Those messages may very well change and we'll be screwed.

[Narretz]

The error messages are still not standardized, therefore we still don't have full support for native classes. Closing.