DI doesn't work with ES6 classes unless the constructor is the first function of the class

[mattlewis92]

• Do you want to request a feature or report a bug?
  Bug
• What is the current behaviour?
  If the constructor doesn't come first in an ES6 class (FYI I'm not using a transpiler, I'm using native ES6 in chrome 49) and there is no explicit annotation then the DI fails and every element is undefined in the constructor e.g.

This works

class test {
  constructor($rootScope) {
    console.log($rootScope);
  }

  test() {}
}
$injector.instantiate(test);

and this doesn't:

class test {

  test() {}

  constructor($rootScope) {
    console.log($rootScope); // undefined
  }
}
$injector.instantiate(test);

• If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem
  http://plnkr.co/edit/K0IGrbT4SPS9g8mkXc5Y?p=preview
• What is the expected behaviour?

The DI to auto inject services

• What is the motivation / use case for changing the behaviour?

The example I gave above is contrived, and normally I would just put the constructor first before any controller methods. However for resolves I often do something like this before a constructor, so you see resolves first and then the constructor where the results are injected:

class MyClass {

  static get resolve() {
    return {
      data: ($http) => $http.get('/path')
    };
  }

  constructor(data) {
    console.log(data);
  }

}

• Which version of Angular, and which browser and OS does this issue affect? Did this work in previous
  versions of Angular? Please also test with the latest stable and snapshot versions.

1.5.0

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

I think the fix should be simple enough by modifying the regex here and look for constructor( before the arguments.

If you need any more info let me know! 😄

[Narretz]

Thanks for the report. I guess it's a bug, even though the use case is pretty rare.

Do you want to open a PR with your suggested fix?

[mattlewis92]

Sure thing, will put something together over the next few days

[lgalfaso]

I think that putting together a fix will be quite difficult without a lot more machinery. Here is a harder example

class Foo {
  test() {
    class Bar {
      constructor() {} // No not pick this one
    }
    "constructor() {}" // nor this one
  }
  "constructor"($rootScope) {} // but this one (as an extra quirk, remember that the function name can be in quotes)
}

and a few other cases that are not supported in Chrome but are in Firefox

class Foo {
  ["const" + "ructor"]($rootScope) {}
}

[dcherman]

@lgalfaso Shouldn't be as bad as you think, although it's probably not that great either (didn't profile this at all).

if (isClass(fn)) {
  fn.toString()
    // Extract the body of the class
    // TODO: Don't match destructuring syntax
    .match(/{(.*)}/)[1]

    // Remove the body of all functions contained within the class
    // so that we don't match inner constructors
    .replace(/{.*?}/g, '')

    // Find the constructor in the list of functions that remain since it may not be
    // the first one in the list
    .match(/constructor\s?\([\s\S]*?\)/)[0]

    // Extract the arguments
    .match(/^[^\(]*\(\s*([^\)]*)\)/m)
}

In Firefox 45B, calling toString on a class constructor doesn't contain the class keyword, so it would continue to follow the non-class DI code. My biggest concern here is the toString() format since I don't think it's defined by the spec (or at least I couldn't find it).

As for your concern about

class Foo {
  ["const" + "ructor"]($rootScope) {}
}

I just tried that in Firefox, and the constructor isn't invoked on new which may be a bug. That said, I think it would be acceptable to say that the injector isn't meant to play hide and seek with your constructor; if you want to do that, use explicit annotation.

Edit

Also, the constructor string would presumably report as the whole output in Chrome rather than the bracket notation, but would need Chrome support to assert that.

[lgalfaso]

@dcherman I think you are looking for http://www.ecma-international.org/ecma-262/6.0/#sec-function.prototype.tostring at toString Representation Requirements

That said, if this can be done reasonable easy, then that is fine with me. The only thing I would not want to end with is with a full ES6 syntax parser to support this.

[gkalpak]

I feel like a RegExps will only take us so far. E.g. @dcherman's will break with something like:

class Foo {
  test() {
    class Bar {
      prop = {};
      constructor() {}
    }
  }
  constructor(arg1, arg2) {}
}

I think we all agree that we don't want to support every wicked usecase.
I suggest looking for the first /constructor\s*\(([^)]*)\)/ match and just have a warning in the docs that inorder for DI to work for un-annotated class constructor, the class' constructor needs to be the first constructor occurence (and not have quotes etc).

[lgalfaso]

I agree that regex can only take us so far, that is why I lean towards
leaving things as-is and updating the docs to state that if automatic DI is
expected, then, for classes, the constructor needs to be defined first

On Monday, March 7, 2016, Georgios Kalpakas [email protected]
wrote:

I feel like a RegExps will only take us so far. E.g. @dcherman
https://github.com/dcherman's will break with something like:

class Foo {
test() {
class Bar {
prop = {};
constructor() {}
}
}
constructor(arg1, arg2) {}
}

I think we all agree that we don't want to support every wicked usecase.
I suggest looking for the first /constructor\s_(([^)]_))/ match and
just have a warning in the docs that inorder for DI to work for
un-annotated class constructor, the class' constructor needs to be the
first constructor occurence (and not have quotes etc).

—
Reply to this email directly or view it on GitHub
#14175 (comment)
.

[dcherman]

Just messing around and I came up with something interesting. I'm really stretching what could be considered reasonable here. This is more because I'm stubborn and wanted to code golf a small solution.

var result = Object.getOwnPropertyNames(fn.prototype).reduce(function(result, name) {
  if (name === 'constructor') {
    return result;
  }
  // Filter on functions in the real implementation too
  return result.replace(fn.prototype[name].toString(), '');
}, fn.toString());

With a reliable Function.prototype.toString that should get you a string containing no other functions but the top level constructor and any properties. Would still fall down in the case of

class Foo {
  prop = {
    constructor(a, b, c) {}
  }
}

Hacky workarounds aside, it seems reasonable to ask consumers to have the first occurrence of constructor be the one they want annotated if they're not going to use ngAnnotate. FWIW, we would still improperly inject stuff in other cases though:

class Foo {
  test() {
    class Bar {
      prop = {};
      constructor(arg1, arg2) {}
    }
  }
}

[aluanhaddad]

Any chance this entire feature can be deprecated in a future release? See #14789 (comment)

[lgalfaso]

You can always enable strictDi, so there I see no need to deprecate this
feature.

On Friday, June 17, 2016, Aluan Haddad [email protected] wrote:

Any chance this entire feature can be deprecated in a future release? See #14789
(comment)
#14789 (comment)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#14175 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AAX44iwnFI8DdRFhrDpIFcZQBZQGo7IZks5qMwg8gaJpZM4Hotu-
.

[aluanhaddad]

Yes, I always use it indeed. The main reason for deprecating would be because it is not recommended. Or is it still considered acceptable practice?

[gkalpak]

Even @johnpapa's styleguide (that you mentioned in the other comment) is suggesting to use this feature (together with ng-annotate) to avoid boilerplate and duplication.

It is an optional feature, very useful for quick examples, POCs and prototypes and you can safely opt-out with strictDi. Together with ng-annotate it is suitable for production and strictDi can help you ensure you are safe.

I don't see any reason to deprecate it tbh.

[mgol]

@gkalpak ng-annotate itself is deprecated, it's superseded by babel-plugin-angularjs-annotate. The recommended mode is with an explicit 'ngInject'; pragma which makes this issue disappear.

I'd like to leave the current behavior as it is. It's impossible to cover all cases and it's way simpler to say that it will always break in non-strict-di mode if constructor with some parameters is not at the top rather than that we have a few heuristics that no one remembers how exactly work and that they were still too weak to detect someone's code. The fact those heuristics are so fragile is the reason why the ng-annotate author recommended strongly the explicit pragma mode. Let's not repeat his original mistakes.

[gkalpak]

@mgol, not sure what you mean. I didn't suggest we change anything (not in this thread at least 😉).
_{(OOC, where does it say ng-annotate is deprecated?)}

[mgol]

I didn't suggest you suggested. ;) I'd like to close this issue with a "Won't fix" resolution.

(OOC, where does it say ng-annotate is deprecated?)

It's not in README yet but see olov/ng-annotate#245 (comment) & olov/ng-annotate#253.

[gkalpak]

I think enough of us have expressed their preference for leaving things as is, that it is reasonable to close this as "won't fix". I'll let you do the honors 😛
Thx everyone for the discussion and feedback!

[mgol]

OK, thanks for the discussion everyone!

[jamie-pate]

as a semi-related issue, this also breaks:

function MakeMixinWith(Ancestor) {
    const
        name = `${Ancestor.name} with MyMixin`;
    return {[name]: class extends Ancestor {
        constructor(...args) {
            super(...args);
            // do something crazy like wrap all async functions with $q
        }
        // some mixin stuff
    }}[name];
}

class BaseClass {}
class ChildClass extends MakeMixinWith(BaseClass) {
        constructor($injectorOrWhatever) {...}
}

('real mixins' as detailed here: http://justinfagnani.com/2015/12/21/real-mixins-with-javascript-classes/ )

but i get the feeling that would also be closed as wontfix

for now the workaround is:

    const MixinWithBaseClass = MakeMixinWith(BaseClass);
    class ChildClass extends MixinWithBaseClass {
        constructor($injectorOrWhatever) {...}
    }