$inject with ES6 classes can inject the wrong dependencies

[ChrisMondok]

Note: for support questions, please use one of these channels: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#question. This repository's issues are reserved for feature requests and bug reports.

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

When angular annotates a base class, it uses the inherited annotations when resolving the dependencies of derived classes.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem via https://plnkr.co or similar (template: http://plnkr.co/edit/tpl:yBpEi4).

http://plnkr.co/edit/55dt9Xx8z5qvHZQ9eDdQ?p=preview

What is the expected behavior?

Derived classes should not inherit their parent's $inject property, as they do not necessarily share dependencies.

What is the motivation / use case for changing the behavior?

This behavior introduces a potentially hard to debug issue, and is probably not what the user expects to happen.

Which versions of Angular, and which browser / OS are affected by this issue? Did this work in previous versions of Angular? Please also test with the latest stable and snapshot (https://code.angularjs.org/snapshot/) versions.

This is still an issue when running against the snapshot as of 2016-12-21

Other information (e.g. stacktraces, related issues, suggestions how to fix)

[dcherman]

How would this play with derived classes that do not define their own constructor? Wouldn't that fail as it would reject the use of the parent's dependencies?

https://plnkr.co/edit/v4S9ViiJIaq9jZwFU1jO?p=preview

[ChrisMondok]

Indeed, the attached PR would break this behavior. Personally, I would consider the fact that this behavior works now is more of a curiosity than a feature. Especially so considering that, if transpiled / minified / under strictDi, the derived class will not be given the base class's $inject property and you'll have an app that works in dev but not in prod.

[ChrisMondok]

Thanks for pointing that out, though, it's definitely worth noting. I'm not sure where that note goes.

[dcherman]

I'm pretty sure it does work on production, as I do exactly this where I work (transpiled through Babel).

[ChrisMondok]

Would you mind adding your comment to my PR ( #15538 ) then? Fixing this issue wouldn't necessarily break your workflow, but my PR probably would.

[dcherman]

Sure, done

[gkalpak]

Here are some thoughts (please correct me if I am wrong or missing something):

Assuming classes Parent and Child extends Parent...

• Currently, the code will break only if all the following conditions are true:

  1. The developer relies on implicit DI annotation.
  2. Parent and Child both have constructors.
  3. Parent and Child have different constructor arguments.

• The solution (from the developer's standpoint) is to properly annotate Child (e.g. via $inject property, inline array, ngAnnotate etc).

• There is no reliable way to detect the lack of constructor in Child. (Is there?)

• If we can't distinguish an inherited constructor from a constructor without arguments, it is not possible to handle implicit annotation correctly on Child.

Based on the above, I would say that there is no way to cover all usecases with implicit annotation, so we need to pick one that won't be supported:

1. Auto-annotating a Child class with a constructor (and different arguments than Parent).
2. Auto-annotating a Child class without a constructor.

Regardless which one we choose not to support, the solution (from the developer's standpoint) will be to explicitly annotate Child classes. Except for one small (but imo important) difference:

If we keep the current implementation (not supporting (1)), the developer needs to explicitly annotate Child with the arguments of its own constructor. If we choose not to support (2), the developer needs to explicitly annotate Child with the arguments of Parent's constructor, which is more tedious and error-prone. A direct result of this difference is that in the first case (current situation), you can use a tool such as ngAnnotate to automatically annotate Child. If we choose to go the other way, then ngAnnotate won't be able to annotate Child correctly.

tl;dr
Unless we can find a (reliable) way to support all usecases, I would rather keep the current implementation and document the limitation as a known issue.

[mprobst]

@gkalpak let's look at the failure modes.

Currently, we silently inject incorrect arguments (for non-strictDi, but also strictDi if users forget to annotate the Child class). This causes very surprising and hard to track down issues – at runtime, long after injection, suddenly something doesn't behave as expected.

If we check hasOwnProperty and require $inject to be on the child class, we avoid this issue. The failure mode is that if Child inherits the constructor from Parent and Parent has been annotated previously (manually or implicitly). The failure mode is that users receive an exception indicating they need to annotate their class.

YMMV, but I'd take explicit errors over unpredictable runtime behaviour anytime.

But maybe we can have our cake and eat it too? If we don't find a constructor in a class definition, we could walk up its Object.getPrototypeOf to handle the case correctly?

On a somewhat unrelated note, looking at the code, it seems to me as if we fail rather surprisingly for classes that don't define an explicit constructor but methods. We use Function.prototype.toString.call(Child), which for a subclass w/o a constructor gives this:

> class Parent { constructor(injected) {} }
> class Child extends Parent { myMethod(a, b) {} }
> Function.prototype.toString.call(Child)
'class Child extends Parent { myMethod(a, b) { } }'

... so AFAICT we'll try to inject a and b into Childs constructor here, which is rather wrong :-/

[gkalpak]

If we check hasOwnProperty and require $inject to be on the child class, we avoid this issue. [...] The failure mode is that users receive an exception indicating they need to annotate their class.

As mentioned in my previous comment, the problem with the proposed solution is that "constructor-less" derived classes will not be annotated correctly. The result will be the equivalent to:

class A { constructor(injected) { this.injected = injected } }
A.$inject = ['injected'];

class B extends A {}
B.$inject = [];   // This will be created implicitly.

In the above example, there will be no error until something tries to access this.injected (which will be undefined). So, the failure will also happen at runtime (although you can argue that debugging undefined may be easier to debug than a defined but incorrect injectable).

Unfortunately, both cases are problematic and quite similar in nature (although they fail on different usecases), but I feel the current one is easier to work around.

Of cource, if we could find a way to cover all bases, that would be great.

But maybe we can have our cake and eat it too? If we don't find a constructor in a class definition, we could walk up its Object.getPrototypeOf to handle the case correctly?

How would we look for a constructor? This issue has come up in the past and there is no easy way out. Ideally we would need a parser, but this would be an overkill, and RegExps/string manipulation isn't robust enough.

Even if we managed to fix it for native classes, this would still be an issue for transpiled code.

On a somewhat unrelated note, looking at the code, it seems to me as if we fail rather surprisingly for classes that don't define an explicit constructor but methods.

I think this is a known limitation of the current implementation, for lack of a reliable way to solve the issue. There is some discussion in #14175.

Just to be clear, I am the first to admit that the current implementation is not ideal and I am open to any sort of improvement.

[mprobst]

Re finding a constructor, currently we fail rather badly... maybe we could:

• check if MyFn.toString() starts with class. If not, do the regular old thing
• if it's a class, find the keyword constructor in the resulting string
  • if constructor is not found at all ==> no constructor, check superclass or if none, zero args
  • if constructor is found, execute the regexp looking for /\s+\([^)]+?\)/ starting from after the constructor keyword, i.e. look for an opening paren immediately following the keyword

That's not super robust – if the keyword constructor occurs anywhere in the class body before the actual constructor, we're likely to fail. But it's a strict improvement over the current state of affairs, isn't it? There is an error case, but this will handle:

• allows detection of classes not defining any constructor (mostly, that is) fixing this issue
• constructor defined after another function or another code snippet that has parentheses
• avoids incorrectly using the arguments to the first method for classes w/o constructors

WDYT?

[mgol]

@mprobst I'd rather our implicit annotations logic expected constructor to be the first method and console.warn or even throw an error otherwise (unless the annotation is explicit). Less surprises; I'm sure we'd break some code if we just looked for constructor somewhere so I'd rather avoid that.