docs(guide/component): fix for component controller unit-test example #14443
Conversation
| @@ -233,9 +233,13 @@ it upwards to the heroList component, which updates the original data. | |||
| </file> | |||
|
|
|||
| <file name="heroDetail.js"> | |||
| function HeroDetailController($scope, $element, $attrs) { | |||
| function HeroDetailController($scope) { | |||
There was a problem hiding this comment.
Do we need $scope here ? Did you leave it on purpose ?
There was a problem hiding this comment.
I made the absolute minimum number of changes to make the unit tests pass. In the unit tests the $componentController is provided with a $scope value, but not with $element or $attrs.
The $scope service is not being used in the controller though, so it might as well be removed.
If I'm not wrong, a PR was merged the other day removing the requirement to always provide $scope in $componentController locals, so it can be removed from there as well.
I can update the PR if you like.
There was a problem hiding this comment.
You are right. Please, do remove the unnecessary $scope and update the test to not provide one.
|
I've removed the Isn't there some way you can get the unit tests to automatically run from docs using dgeni? It's already parsing the code snippets and putting them into files. Maybe it's possible to generate a karma config and run tests as well. |
|
We are running the e2e tests from the docs. Not sure if we are already doing the same for the guide. |
| @@ -464,8 +468,7 @@ describe('component: heroDetail', function() { | |||
| })); | |||
|
|
|||
| it('should set the default values of the hero', function() { | |||
| // It's necessary to always pass the scope in the locals, so that the controller instance can be bound to it | |||
| component = $componentController('heroDetail', {$scope: scope}); | |||
| component = $componentController('heroDetail', {}); | |||
There was a problem hiding this comment.
Nitpick: I prefer to omit the unnecessary locals argument altogether (or pass null where we want a 3rd argument).
|
Except for the fact that I am not a big fan of the initialization logic (see https://github.com/angular/angular.js/pull/14443/files#r60963112), it LGTM. |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Docs update.
What is the current behavior? (You can also link to an open issue here)
Error in docs is described in #14426.
Does this PR introduce a breaking change?
No.
Please check if the PR fulfills these requirements