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

feat(ngMock.$componentController): add helper to instantiate controllers for components #13711

Closed

Conversation

petebacondarwin
Copy link
Member

Closes #13683

expect(ctrl).toEqual({$scope: $scope, a: 'A', b: 'B', x: 'X', y: 'Y' });
});
});
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add a test with controller: 'TestController as test'

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you like but the implementation is just proxying to $controller anyway so it doesn't test any new paths.

@Narretz
Copy link
Contributor

Narretz commented Jan 9, 2016

Wouldn't it be nice to have that for plain directives, too?

@gkalpak
Copy link
Member

gkalpak commented Jan 9, 2016

There will be some slight inconsistency between using controller: 'SomeCtrl as identifier' and controller: 'SomeCtrl' or controller: SomeCtrl. In the former case, the controller will be added as an alias on the scope, whereas in other two cases, it won't (when usign $componentController, I mean). Likewise, failing to provide a $scope in locals will throw an error in the former case (because it will try to assign the controller instance on the scope), but not in the other two cases.

@petebacondarwin
Copy link
Member Author

@Narretz - it is much more complicated to do it for plain directives because we don't actually call the directive factory function until the directive is matched in some HTML. This means that the controller is not easily available. Components are simpler because the controller is not wrapped in a factory function.

@petebacondarwin
Copy link
Member Author

There will be some slight inconsistency between using controller: 'SomeCtrl as identifier' and controller: 'SomeCtrl' or controller: SomeCtrl. In the former case, the controller will be added as an alias on the scope, whereas in other two cases, it won't (when usign $componentController, I mean). Likewise, failing to provide a $scope in locals will throw an error in the former case (because it will try to assign the controller instance on the scope), but not in the other two cases.

@gkalpak this is also true of ngMock.$controller right now.

@petebacondarwin petebacondarwin force-pushed the componentController branch 2 times, most recently from 2d031d6 to 0fa49cd Compare January 9, 2016 16:21
@gkalpak
Copy link
Member

gkalpak commented Jan 10, 2016

@petebacondarwin, true, but things are more explicit there. You are are the one passing the argument to $controller() so the outcome should be less surprizing. In the case of $componentController() this happens indirectly; I would expect the same outcome when instantiating a component controller.

I wouldn't go to great lengths trying to "fix" this. If there's no easy fix (e.g. extracting the controller name and ignoring the alias or something), I would at least document that when using controller: 'SomeCtrl as identifier', you need to supply a $scope in locals.

No biggie anyway - it only affects test code. Feel free to leave as is, if you think it's OK.
LGTM otherwise 👍

@petebacondarwin
Copy link
Member Author

@gkalpak - OK, I see what you mean. But since this is for people testing controllers that (hopefully) they have actually written, then it shouldn't be too surprising!

@petebacondarwin
Copy link
Member Author

Actually in light of the latest discussions about controller as we should probably provide the ident param, if it is not explicitly provided, to be in line with the values of the component definition. I.E. if none is given the controller is attached to the scope via $scope.$ctrl.

@gkalpak
Copy link
Member

gkalpak commented Jan 10, 2016

Now it LRGTM ('R' for "really") 😃

@petebacondarwin petebacondarwin deleted the componentController branch November 24, 2016 09:04
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.

None yet

4 participants