Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 9 additions & 21 deletions src/auto/injector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -821,17 +821,6 @@ function createInjector(modulesToLoad, strictDi) {
return args;
}

function isClass(func) {
// IE 9-11 do not support classes and IE9 leaks with the code below.
if (msie <= 11) {
return false;
}
// Workaround for MS Edge.
// Check https://connect.microsoft.com/IE/Feedback/Details/2211653
return typeof func === 'function'
&& /^(?:class\s|constructor\()/.test(Function.prototype.toString.call(func));
}

function invoke(fn, self, locals, serviceName) {
if (typeof locals === 'string') {
serviceName = locals;
Expand All @@ -843,15 +832,16 @@ function createInjector(modulesToLoad, strictDi) {
fn = fn[fn.length - 1];
}

if (!isClass(fn)) {
try {
// http://jsperf.com/angularjs-invoke-apply-vs-switch
// #5388
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// if `fn` is a class, then it needs to be called using `new`
if (e instanceof TypeError) {
return new (Function.prototype.bind.apply(fn, [null].concat(args)))();
} else {
throw e;
}
}
}

Expand All @@ -863,9 +853,7 @@ function createInjector(modulesToLoad, strictDi) {
var args = injectionArgs(Type, locals, serviceName);
// Empty object at position 0 is ignored for invocation with `new`, but required.
args.unshift(null);
/*jshint -W058 */ // Applying a constructor without immediate parentheses is the point here.
return new (Function.prototype.bind.apply(ctor, args));
/*jshint +W058 */
return new (Function.prototype.bind.apply(ctor, args))();
}


Expand Down
3 changes: 2 additions & 1 deletion test/ng/compileSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
'use strict';

/* globals support: false */

describe('$compile', function() {
function isUnknownElement(el) {
Expand Down Expand Up @@ -4241,7 +4242,7 @@ describe('$compile', function() {


it('should eventually expose isolate scope variables on ES6 class controller with controllerAs when bindToController is true', function() {
if (!/chrome/i.test(navigator.userAgent)) return;
if (!support.classes) return;
/*jshint -W061 */
var controllerCalled = false;
module(function($compileProvider) {
Expand Down