Injector incorrectly infers injectables for ES6 classes

[TEHEK]

TL;DR: Using ES6 classes and Angular has a huge caveat. A temp workaround is to:

• always declare constructor
• declare constructor first

extractArgs method relies on a regular expression to extract function arguments. However when instantiating an ES6 class, the regexp may fail. E.g.:

https://jsfiddle.net/ykmtej1j/

vs

https://jsfiddle.net/ykmtej1j/1/

In the first example the extractArgs incorrectly infers method(wrongArgument) as constructor (it's only looking for opening parenths).

Regular expression is not safe to use with ES6 classes. And unless there's a way to extract the constructor method at runtime, extractArgs would need a real JS AST parser to correctly and reliably infer constructor arguments.

[TEHEK]

Looks like the issue does not occur in Firefox.

class Cls {
  constructor($http) { }
  method(a) { }
}

console.log(Cls.toString())

prints

function Cls($http) {
"use strict";
 }

[mprobst]

@petebacondarwin could you TAL? @TEHEK also has an idea on how to fix, maybe share it here?

[TEHEK]

Sure.

In ES6 class declaration constructor is a method declaration within the top class { ... } level. Which means the task at hand doesn't require the real AST parser per se and a simple lexer should do. It needs to correctly process:

• quoted strings (", ') with escaping
• templated strings
• comments (//, /*)
• count opening and closing "{" and "}" (and maybe parenthesis as well)

It can assume the input syntax is valid since browser takes care of that.

[petebacondarwin]

Here are some related issues: #14175, #13785, #14531, #14533
Right now different browsers stringify classes in different ways, so even if we could write a suitable parser it would not work across the board.

[TEHEK]

That doesn't matter. If the fn.toString() returns something that starts with "class ... { ... }", it could be parsed.

Firefox returns the constructor function declaration, so the old injector works just fine.

[petebacondarwin]

But once they sort themselves out, then some micro-parser would be great.

[TEHEK]

... unless Chrome behavior is going to change to match others in which case it would be just a wasted effort :)

[gkalpak]

This is essentially a known issue (e.g. see #14175 (comment)).
Considering that:

• (a) using a lexer just for this would be an overkill (and it would still fail in certain corner-cases, I think)
• (b) browsers are inconsistent, buggy and probable to change their implementations in the near future
• (c) this only affects a dev-time feature (automagic DI)
• (d) it is easy to work-around this by using a different (proper) DI method

I think we should just document it as a known issue for now.
We can re-visit later, when browser implementations have stabilized.

BTW, Firefox's behavior is violating the spec and causing another bug (when trying to determine whether to pre-assign bindings on a directive controller).

[Narretz]

Where should this be documented?

[gkalpak]

Here and there, I guess.
But do others agree that we should just document it for now ?

[petebacondarwin]

Given that there is a very reasonable workaround (to annotate the class), which is actually what you would expect to do in production anyway, then I am happy to go with the documentation option for now.

@TEHEK would you like to comment?

[TEHEK]

Well, i guess documenting the caveat is reasonable. The magic auto-injection is what makes angular so attractive in the first place. I'll keep the microparser in my backlog just in case.

[petebacondarwin]

@TEHEK - thanks. Perhaps an alternative place to look to add this micro-parser is the ng-annotate project?