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 notifications@github.com
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) {}
    }
  }
}