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

fix($injector): add workaround for class stringification in Chrome v50/51 #14531

Closed
wants to merge 1 commit into from
Closed

fix($injector): add workaround for class stringification in Chrome v50/51 #14531

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
10 changes: 7 additions & 3 deletions src/auto/injector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,12 +70,16 @@ var FN_ARG = /^\s*(_?)(\S+?)\1\s*$/;
var STRIP_COMMENTS = /((\/\/.*$)|(\/\*[\s\S]*?\*\/))/mg;
var $injectorMinErr = minErr('$injector');

function extractArgs(fn) {
function stringifyFn(fn) {
// Support: Chrome 50-51 only
// Creating a new string by adding `' '` at the end, to hack around some bug in Chrome v50/51
// (See https://github.com/angular/angular.js/issues/14487.)
// TODO (gkalpak): Remove workaround when Chrome v52 is released
var fnText = Function.prototype.toString.call(fn).replace(STRIP_COMMENTS, '') + ' ',
return Function.prototype.toString.call(fn) + ' ';
}

function extractArgs(fn) {
var fnText = stringifyFn(fn).replace(STRIP_COMMENTS, ''),
args = fnText.match(ARROW_ARG) || fnText.match(FN_ARGS);
return args;
}
Expand Down Expand Up @@ -849,7 +853,7 @@ function createInjector(modulesToLoad, strictDi) {
if (!isBoolean(result)) {
// Workaround for MS Edge.
// Check https://connect.microsoft.com/IE/Feedback/Details/2211653
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you could change the comment at the same time to:

// Support: Edge 12-13 only
// See https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/6156135/

It's fixed in the Edge preview and the bug has moved.

Copy link
Member Author

Choose a reason for hiding this comment

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

Too late 😛
Will do in a follow-up PR.

result = func.$$ngIsClass = /^(?:class\s|constructor\()/.test(Function.prototype.toString.call(func));
result = func.$$ngIsClass = /^(?:class\s|constructor\()/.test(stringifyFn(func));
Copy link
Contributor

Choose a reason for hiding this comment

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

In the regex, let's replace \s with \b. Otherwise it doesn't work in the situations like following:

  1. class{ ... }
  2. class/* foo */ Bar { ... }

Copy link
Member Author

Choose a reason for hiding this comment

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

I meant to do it as part of this PR, but forgot.
I will submit a new one.

}
return result;
}
Expand Down
21 changes: 21 additions & 0 deletions test/auto/injectorSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,14 @@ describe('injector', function() {
it('should take args before first arrow', function() {
expect(annotate(eval('a => b => b'))).toEqual(['a']);
});

// Support: Chrome 50-51 only
Copy link
Contributor

Choose a reason for hiding this comment

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

+1 :-)

// TODO (gkalpak): Remove when Chrome v52 is relased.
// it('should be able to inject fat-arrow function', function() {
// inject(($injector) => {
// expect($injector).toBeDefined();
// });
// });
}

if (support.classes) {
Expand All @@ -293,6 +301,19 @@ describe('injector', function() {
expect(instance).toEqual(new Clazz('a-value'));
expect(instance.aVal()).toEqual('a-value');
});

// Support: Chrome 50-51 only
// TODO (gkalpak): Remove when Chrome v52 is relased.
// it('should be able to invoke classes', function() {
// class Test {
// constructor($injector) {
// this.$injector = $injector;
// }
// }
// var instance = injector.invoke(Test, null, null, 'Test');

// expect(instance.$injector).toBe(injector);
// });
}
/*jshint +W061 */
});
Expand Down