fix($injector): Allows ES6 function syntax #12425
Conversation
|
BTW, the travis failures are because the Chrome and Firefox versions running on Travis do not support this syntax. The test do work with Chrome 44 and Firefox 39. |
| if (/chrome|firefox/i.test(navigator.userAgent)) { | ||
| 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']); |
There was a problem hiding this comment.
Can you explain this better? How does it break?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @@ -237,6 +237,15 @@ describe('injector', function() { | |||
| }); | |||
|
|
|||
|
|
|||
| // Only Chrome and Firefox support this syntax. | |||
| if (/chrome|firefox/i.test(navigator.userAgent)) { | |||
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
I know.
BTW, it also has Safari and (of course) Edge (for interoperability reasons - or so I've read :P).
Closes #12424