feat($injector): support instantiating classes. #12598

mprobst · 2015-08-17T11:30:19Z

ES6's class Foo {} constructors cannot be instantiated using
fn.apply. This change extracts injection argument collection and then
uses new (Function.bind.apply(ctor, args)) to instantiate the service
instance.

Fixes #12597.

mprobst · 2015-08-17T11:38:55Z

Note: this change requires using Function.bind, which is quite a bit slower in instantiating objects in a synthetic benchmark (x20). However I wouldn't expect that the instantiate code flow is very critical - it's usually only used for services and controllers, both of which should be instantiated relatively rarely in a typical application's lifecycle.

IgorMinar · 2015-08-17T16:03:16Z

tests are failing

mprobst · 2015-08-17T16:11:30Z

@IgorMinar should be fixed (FF doesn't enable class syntax by default).

IgorMinar · 2015-08-19T17:49:52Z

test/auto/injectorSpec.js

+          var clazz = eval('(class { constructor(a) { this.a = a; } aVal() { return this.a; } })');
+          var instance = injector.instantiate(clazz);
+          expect(instance).toEqual({a: 'a-value'});
+          // Check if prototype is set up correctly.s


you are not checking the prototype + there is a typo at the end of the comment. I'd just remove this.

Well, the call to aVal() would fail if the prototype was incorrect. But point taken, dropped the comment.

mprobst · 2015-08-20T08:19:09Z

PTAL.

Narretz · 2015-08-21T10:17:48Z

@mprobst While you are at it, could you check this related issue: #12614? If $controller uses $injector, shouldn't this PR make it work?

mprobst · 2015-08-21T11:16:46Z

#12614 is not fixed by this – $controller uses $injector, but bindToController has its own logic to instantiate controllers on directives. I'm not sure it'll be possible to fix bindToController, it's way of instantiating a class on top of an existing object, then applying the ctor to it might be inherently incompatible with ES6.

caitp · 2015-08-21T13:08:48Z

then applying the ctor to it might be inherently incompatible with ES6

It generally is, but this might not be too much of a problem for much longer. It's likely that the "new-only" restriction will be lifted, or otherwise a way of specializing a "call" behaviour/factory method for classes.

In the mean time, you could add some other compat mechanism, like falling back on a static "init()" method if calling the constructor fails. There's no reason this couldn't be solved using conventions rather than language features

mprobst · 2015-08-21T13:27:49Z

@caitp: indeed, but what about doing that in another PR?

caitp · 2015-08-21T13:51:07Z

Yeah, doesn't need to be this one, although that feature probably shouldn't be specific to controllers if we do that

stefan-- · 2015-08-30T18:17:46Z

when can we expect this to work?

mprobst · 2015-08-31T07:51:31Z

@IgorMinar PTAL, I believe this is good to merge. It does not solve the issue for controllerAs yet, but that's more tricky (and may or may not be impossible).

IgorMinar · 2015-09-03T17:32:27Z

lgtm. @petebacondarwin should we wait with a merge until master is open for 1.5 beta features?

IgorMinar · 2015-09-03T17:33:21Z

@petebacondarwin, one more thing - this feature is not documented at all. where should the documentation live?

petebacondarwin · 2015-09-06T19:54:56Z

src/auto/injector.js

-      for (i = 0, length = $inject.length; i < length; i++) {
-        key = $inject[i];
+      for (var i = 0, length = $inject.length; i < length; i++) {
+        var key = $inject[i];


Why move the declarations of i and key here? Being var declarations they will still be hoisted to the top of the function block.

Also we seem to have lost the declaration of the length variable here.

length is delayed as part of the var declaration

Declared, darned Swype

@petebacondarwin I was moving the code around anyway, and I personally find it easier to declare things as close as possible to their respective use site, hoisting or not. Makes it easier to see what's used where and why, and also makes it easier to refactor code without forgetting to remove declarations.

petebacondarwin · 2015-09-06T20:02:34Z

However I wouldn't expect that the instantiate code flow is very critical - it's usually only used for services and controllers, both of which should be instantiated relatively rarely in a typical application's lifecycle.

I am not sure about this statement. Controllers can be instantiated very often. Every instance of a directive which defines a "directive controller" would need to instantiate a directive. Also imagine if you use ng-controller inside a frequently changing ng-repeat block.

petebacondarwin · 2015-09-06T20:04:14Z

I guess the documentation for this should be in the $injector class: https://docs.angularjs.org/api/auto/service/$injector

mprobst · 2015-09-07T06:29:12Z

@petebacondarwin absent a good benchmark, this is all guesswork. However I would imagine the cost of this particular call being dwarfed by the cost of both ng-repeat and the DOM manipulations for directive controllers or ng-controller in a loop. I'd expect you can construct a synthetic benchmark that does slow down, but I wouldn't expect actual apps experiencing that.

ES6's `class Foo {}` constructors cannot be instantiated using `fn.apply`. This change extracts injection argument collection and then uses new (Function.bind.apply(ctor, args)) to instantiate the service instance.

mprobst · 2015-10-26T09:18:07Z

Friendly ping?

IgorMinar · 2015-12-10T02:04:32Z

@petebacondarwin is this too high risk for 1.5? several people at Google are asking for this change.

IgorMinar · 2015-12-10T02:06:20Z

@lgalfaso - what do you think?

lgalfaso · 2015-12-10T18:17:38Z

@IgorMinar it mostly moves code around, and it adds a little magic but it is reasonably documented. From my point of view, it looks safe for 1.5. If @petebacondarwin does not oppose, then I will merge it EOD Friday.

petebacondarwin · 2015-12-10T20:09:23Z

Go for it @lgalfaso

bisubus · 2015-12-10T20:20:50Z

@petebacondarwin It looks like the commit does not affect later argument in $controller which still uses $injector.invoke.

IgorMinar · 2015-12-10T20:52:28Z

sgtm

On Thu, Dec 10, 2015 at 12:21 PM bisubus [email protected] wrote:

@petebacondarwin https://github.com/petebacondarwin It looks like the
commit does not affect later argument

angular.js/src/ng/controller.js

Line 117 in 62d514b

if (later) {

in $controller which still uses $injector.invoke.

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

petebacondarwin · 2015-12-11T09:42:03Z

@petebacondarwin It looks like the commit does not affect later argument in $controller which still uses $injector.invoke.

@bisubus - thanks for pointing that out.
Can we create a new issue for that case so that we can track it and work on fixing it?

petebacondarwin · 2015-12-11T09:45:50Z

I have created the issue at #13510

googlebot added the cla: yes label Aug 17, 2015

mprobst added the component: di label Aug 17, 2015

mprobst force-pushed the es6-instantiate branch from 1ea5fec to df99769 Compare August 17, 2015 16:09

trevorade mentioned this pull request Aug 18, 2015

$controller cannot instantiate ES6 classes #12614

Closed

IgorMinar reviewed Aug 19, 2015
View reviewed changes

mprobst force-pushed the es6-instantiate branch from df99769 to 94ec404 Compare August 20, 2015 08:19

mprobst assigned IgorMinar Aug 31, 2015

mprobst force-pushed the es6-instantiate branch from 94ec404 to bb94223 Compare August 31, 2015 07:51

mprobst force-pushed the es6-instantiate branch 2 times, most recently from f27e77e to e0567ce Compare August 31, 2015 09:23

petebacondarwin reviewed Sep 6, 2015
View reviewed changes

feat($injector): support instantiating classes.

06ecb4b

ES6's `class Foo {}` constructors cannot be instantiated using `fn.apply`. This change extracts injection argument collection and then uses new (Function.bind.apply(ctor, args)) to instantiate the service instance.

mprobst force-pushed the es6-instantiate branch from e0567ce to 06ecb4b Compare September 28, 2015 11:34

bisubus mentioned this pull request Oct 20, 2015

$injector cannot instantiate ES6 classes #12597

Closed

mprobst closed this in 8b6b428 Dec 10, 2015

petebacondarwin mentioned this pull request Dec 11, 2015

fix($controller): support instantiation of ES6 classes #13510

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat($injector): support instantiating classes. #12598

feat($injector): support instantiating classes. #12598

mprobst commented Aug 17, 2015

mprobst commented Aug 17, 2015

IgorMinar commented Aug 17, 2015

mprobst commented Aug 17, 2015

IgorMinar Aug 19, 2015

mprobst Aug 20, 2015

mprobst commented Aug 20, 2015

Narretz commented Aug 21, 2015

mprobst commented Aug 21, 2015

caitp commented Aug 21, 2015

mprobst commented Aug 21, 2015

caitp commented Aug 21, 2015

stefan-- commented Aug 30, 2015

mprobst commented Aug 31, 2015

IgorMinar commented Sep 3, 2015

IgorMinar commented Sep 3, 2015

petebacondarwin Sep 6, 2015

petebacondarwin Sep 6, 2015

caitp Sep 6, 2015

caitp Sep 6, 2015

mprobst Sep 7, 2015

petebacondarwin commented Sep 6, 2015

petebacondarwin commented Sep 6, 2015

mprobst commented Sep 7, 2015

mprobst commented Oct 26, 2015

IgorMinar commented Dec 10, 2015

IgorMinar commented Dec 10, 2015

lgalfaso commented Dec 10, 2015

petebacondarwin commented Dec 10, 2015

bisubus commented Dec 10, 2015

IgorMinar commented Dec 10, 2015

petebacondarwin commented Dec 11, 2015

petebacondarwin commented Dec 11, 2015

feat($injector): support instantiating classes. #12598

feat($injector): support instantiating classes. #12598

Conversation

mprobst commented Aug 17, 2015

mprobst commented Aug 17, 2015

IgorMinar commented Aug 17, 2015

mprobst commented Aug 17, 2015

IgorMinar Aug 19, 2015

Choose a reason for hiding this comment

mprobst Aug 20, 2015

Choose a reason for hiding this comment

mprobst commented Aug 20, 2015

Narretz commented Aug 21, 2015

mprobst commented Aug 21, 2015

caitp commented Aug 21, 2015

mprobst commented Aug 21, 2015

caitp commented Aug 21, 2015

stefan-- commented Aug 30, 2015

mprobst commented Aug 31, 2015

IgorMinar commented Sep 3, 2015

IgorMinar commented Sep 3, 2015

petebacondarwin Sep 6, 2015

Choose a reason for hiding this comment

petebacondarwin Sep 6, 2015

Choose a reason for hiding this comment

caitp Sep 6, 2015

Choose a reason for hiding this comment

caitp Sep 6, 2015

Choose a reason for hiding this comment

mprobst Sep 7, 2015

Choose a reason for hiding this comment

petebacondarwin commented Sep 6, 2015

petebacondarwin commented Sep 6, 2015

mprobst commented Sep 7, 2015

mprobst commented Oct 26, 2015

IgorMinar commented Dec 10, 2015

IgorMinar commented Dec 10, 2015

lgalfaso commented Dec 10, 2015

petebacondarwin commented Dec 10, 2015

bisubus commented Dec 10, 2015

IgorMinar commented Dec 10, 2015

petebacondarwin commented Dec 11, 2015

petebacondarwin commented Dec 11, 2015