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

fix($injector): Allows ES6 function syntax #12425

Closed
wants to merge 1 commit into from
Closed

fix($injector): Allows ES6 function syntax #12425

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
2 changes: 1 addition & 1 deletion src/auto/injector.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@
* Implicit module which gets automatically added to each {@link auto.$injector $injector}.
*/

var FN_ARGS = /^function\s*[^\(]*\(\s*([^\)]*)\)/m;
var FN_ARGS = /^[^\(]*\(\s*([^\)]*)\)/m;
var FN_ARG_SPLIT = /,/;
var FN_ARG = /^\s*(_?)(\S+?)\1\s*$/;
var STRIP_COMMENTS = /((\/\/.*$)|(\/\*[\s\S]*?\*\/))/mg;
Expand Down
9 changes: 9 additions & 0 deletions test/auto/injectorSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,15 @@ describe('injector', function() {
});


// Only Chrome and Firefox support this syntax.
if (/chrome|firefox/i.test(navigator.userAgent)) {
Copy link
Member

Choose a reason for hiding this comment

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

This regex will also match MS Edge's navigator (but it's OK, since it understands the syntax as well).
(But the comment is misleading :P)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I do not have access to MS Edge and it is currently not supported by saurcelabs, so this will have to make it.

On a side note, I find it somehow strange for MS Edge user agent to have the word "Chrome" in it

Copy link
Member

Choose a reason for hiding this comment

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

I know.
BTW, it also has Safari and (of course) Edge (for interoperability reasons - or so I've read :P).

it('should be possible to annotate functions that are declared using ES6 syntax', function() {
// The function is generated using `eval` as just having the ES6 syntax can break some browsers.
expect(annotate(eval('({ fn(x) { return; } })').fn)).toEqual(['x']);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain this better? How does it break?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I change the test to just

expect(annotate(({ fn(x) { return; } }).fn)).toEqual(['x']);

then the JS parser complains about the syntax. It claims there is a missing : after fn and stops

Copy link
Contributor

Choose a reason for hiding this comment

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

Which JS parser? What browser versions are you talking about? Are you talking about the test file failing to parse in older browsers? Or what?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without the eval trick, Safari 8.0.7 (10600.7.12) [This is the latest Safari version]
throws this error when parsing the file to run the tests

SyntaxError: Unexpected token '('. Expected a ':' following the property name 'fn'.

After throwing this error, no tests are executed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, I'd rather we jUst put the test in a different source file which is only loaded in known supported browsers, because I really don't want to rely on eval. Take your pick and document clearly the approach you've taken. Otherwise LGTM

});
}


it('should publish annotate API', function() {
expect(angular.mock.$$annotate).toBe(annotate);
spyOn(angular.mock, '$$annotate').andCallThrough();
Expand Down