Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(ngMock#$controller): properly assign bindings to all types of controllers (e.g. class-based) #14439

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Apr 14, 2016

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix (I think).

What is the current behavior? (You can also link to an open issue here)
The bindings are not assigned to the final instances of controllers specified as Classes or returning a value from their constructor.
See #14437.

What is the new behavior (if this is a feature change)?
The bindings are assigned to any type of controller, similar to how $compile would do it.

Does this PR introduce a breaking change?
Not really.

Please check if the PR fulfills these requirements

Other information:
The decorated version of $controller is able to assign bindings to a controller instance prior to
instantiation, emulating the behavior of $compile with directive controllers.

There are cases, that the actual controller instance is different than the pre-populated one (e.g.
when the controller constructor function returns a value or when the controller is an ES2015 Class).
While $compile accounts for such situation, by re-asigning the bindings after the controller has
been instantiated, ngMock's $controller didn't.

This commit fixes it, by re-applying the bindings if the actual controller instance is different
than the original one (similar to how $compile would do it).

Fixes #14437

@Narretz
Copy link
Contributor

Narretz commented Apr 15, 2016

Safari 9 doesn't like this:

Safari 9.0.0 (Mac OS X 10.11.0) ngMock $controllerDecorator should support assigning bindings to class-based controller FAILED

    TypeError: Cannot call a class constructor (line 1)

    invoke@/home/travis/build/angular/angular.js/src/auto/injector.js:867:24

    $controllerInit@/home/travis/build/angular/angular.js/src/ng/controller.js:147:40

    /home/travis/build/angular/angular.js/src/ngMock/angular-mocks.js:2187:33

    /home/travis/build/angular/angular.js/test/ngMock/angular-mocksSpec.js:1933:33

    invoke@/home/travis/build/angular/angular.js/src/auto/injector.js:867:24

    workFn@/home/travis/build/angular/angular.js/src/ngMock/angular-mocks.js:2965:26

    inject@/home/travis/build/angular/angular.js/src/ngMock/angular-mocks.js:2934:46

    /home/travis/build/angular/angular.js/test/ngMock/angular-mocksSpec.js:1932:15

    inject@/home/travis/build/angular/angular.js/src/ngMock/angular-mocks.js:2927:34

    /home/travis/build/angular/angular.js/test/ngMock/angular-mocksSpec.js:1932:15

@Narretz
Copy link
Contributor

Narretz commented Apr 15, 2016

We can test on 9.1 though, maybe it was fixed by this version( I doubt it)

@gkalpak
Copy link
Member Author

gkalpak commented Apr 15, 2016

Yeah, I saw it failed. I pushed a potential fix, but it failed again and I'm not sure why (I can't test locally on Windows).

I'll try to track this down via SauceLabs' manual testing later (or tomorrow).

@Narretz
Copy link
Contributor

Narretz commented Apr 15, 2016

Isn't that the same problem with have in compile with Safari?

@gkalpak
Copy link
Member Author

gkalpak commented Apr 15, 2016

No idea :)

@Narretz
Copy link
Contributor

Narretz commented Apr 15, 2016

I was thinking about this: #13785

@gkalpak
Copy link
Member Author

gkalpak commented Apr 15, 2016

@Narretz, it seems like it. So, does it mean that $compile won't currently work with class-based controllers on Safari 8/9.0.0 ?

…trollers (e.g. class-based)

The decorated version of `$controller` is able to assign bindings to a controller instance prior to
instantiation, emulating the behavior of `$compile` with directive controllers.

There are cases, that the actual controller instance is different than the pre-populated one (e.g.
when the controller constructor function returns a value or when the controller is an ES2015 Class).
While `$compile` accounts for such situation, by re-asigning the bindings after the controller has
been instantiated, `ngMock`'s `$controller` didn't.

This commit fixes it, by re-applying the bindings if the actual controller instance is different
than the original one (similar to how `$compile` would do it).

Fixes angular#14437
@gkalpak gkalpak force-pushed the fix-componentController-assign-bindings-on-Class branch from eb55a07 to 974cad9 Compare April 15, 2016 18:19
@gkalpak
Copy link
Member Author

gkalpak commented Apr 15, 2016

That is indeed the case. I changed the class-based test to only run on Chrome (we are doing the same in the $compile tests).

@drpicox
Copy link
Contributor

drpicox commented Apr 16, 2016

I'm thinking that may be it is also related to: #14206

@petebacondarwin petebacondarwin modified the milestones: 1.5.5, 1.5.6 Apr 18, 2016
@Narretz Narretz modified the milestones: 1.5.6, 1.5.7 May 27, 2016
@Narretz
Copy link
Contributor

Narretz commented Jun 7, 2016

LGTM

@gkalpak gkalpak closed this in 96266b9 Jun 7, 2016
gkalpak added a commit that referenced this pull request Jun 7, 2016
…trollers (e.g. class-based)

The decorated version of `$controller` is able to assign bindings to a controller instance prior to
instantiation, emulating the behavior of `$compile` with directive controllers.

There are cases, that the actual controller instance is different than the pre-populated one (e.g.
when the controller constructor function returns a value or when the controller is an ES2015 Class).
While `$compile` accounts for such situation, by re-asigning the bindings after the controller has
been instantiated, `ngMock`'s `$controller` didn't.

This commit fixes it, by re-applying the bindings if the actual controller instance is different
than the original one (similar to how `$compile` would do it).

Fixes #14437

Closes #14439
@gkalpak gkalpak deleted the fix-componentController-assign-bindings-on-Class branch June 7, 2016 08:19
@villelahdenvuo
Copy link

I still seem to have this issue with $componentController and angular 1.5.8 and ES6 classes, neither locals or bindings are being set on the controller instance.

@mgol
Copy link
Member

mgol commented Sep 21, 2016

@tuhoojabotti It works fine for me, I have multiple tests that rely on that. If something's broken for you, please open a new issue.

@villelahdenvuo
Copy link

@mgol I just realized it's because of my test setup. I do not use ngMock because I can't get past calling flush on every http request, so I have my own mock module that exposes some ngMock providers, but I forgot to add the $controllerDecorator, that's why the bindings aren't being set.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

$componentController doesn't bind properties when component uses ES6 controller class
7 participants