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?

[petebacondarwin]

Sorry, didn't mean to close this until we have the documentation sorted.

[aluanhaddad]

Magic auto-injection is a bad feature that should never have been there in the first place. In what other context does the name of a parameter influence anything. Angular's magic DI system uses the name to infer the type, but one should be able to call a parameter anything they would like. When I started using angular, over 3 years ago, I was very confused by this behavior. At the time I didn't know how angular knew what to inject. I did some research and found out it was using Function.protoype.toString (yuck!) and that there were other ways. I never used the magic way again.

That was a long time ago and much has changed for the better. The docs use better examples, mostly, and a number of well reasoned patterns and practices have become mainstream thanks to people like @johnpapa.

I think it's time to deprecate this feature.

[TEHEK]

Injection mechanism is already different in Angular2. Ditching existing users by deprecating a core feature is a no-go, imo.

Users have a reasonable expectation that injector should just work when, for example, they migrate to ES6 classes.

Yes injection magic is not ideal, but at least it was consistent.

[aluanhaddad]

I am saying that it should be deprecated in the next version. If you want to deploy your app, you have to rewrite you code or run it through a tool.

[TEHEK]

Unless the feature is going away, there's no point in adding a useless deprecation warning. And i think removing auto-injection is a major breaking change not suitable for 1.x branch.

[gkalpak]

It is not productive to have this conversation in two places. Let's continue in #14175.

[Narretz]

Let's close as a duplicate then