feat(ngMock.$componentController): not increase extra bytes in angular.min.js

[drpicox]

I made some changes to the solution of @petebacondarwin that closes #13683
Basically:

• I have removed the $$componentControllers map from $controlerProvider
• I have used $injector to get the component directive structure

The objective of these changes are:

• do not increase extra bytes the angular.min.js

[googlebot]

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

[petebacondarwin]

Nice one @drpicox - I will tweak this (due to refactorings I did already) and merge it on top of what I just landed.

[drpicox]

Thanks, and thanks also for your code! (Oh, that's the reason of conflicts)
That was the kind of implementation that I had in mind, it's not the better,

• ¿what happens if there are multiple directives with the same name?
• ¿is it ok allow to use it on directives?

Btw, I do not understand why says cla:no, I have signed it on Jun 06, 2015 04:43 PDT

It looks like you've already signed this CLA. If you'd like to edit your contact information, you may do so below.

Date Signed
Jun 06, 2015 04:43 PDT

[petebacondarwin]

It says no because there are commits from you and me; and it is wants to check that you aren't just pushing someone else's code without their permission

[petebacondarwin]

We can't use this on directives because the injector will only return a factory function for directives.

[petebacondarwin]

One can only have a single component directive on an element, so having multiple component directives with the same name would not make sense.

I am not sure what the injector will do if there are multiple directives with the same name.

[drpicox]

So it seems that's ok.
Thanks.

[gkalpak]

The problem with this change is that it can't defferentiate between components and non-component directives and assumes.

If there are non-component directives that have the same name as a component, then the order of registration will affect the behavior of $componentController (and that's not good).

Maybe we could add a private (as in $$-prefixed) property on components, so we can tell them apart.

[petebacondarwin]

I agree that there are some potential issues with this approach. Let me have a play today.

[drpicox]

@gkalpak it is what I expected, but imho components are directives (sugar syntax), in fact they are registered as directives:

angular.js/src/ng/compile.js

Line 1092 in dd14e0c

return this.directive(name, factory);

 this.component = function registerComponent(name, options) {
    ...
    function factory($injector) {
      ...
      return {
        controller: controller,
        controllerAs: ident,
        template: makeInjectable(template),
        templateUrl: makeInjectable(options.templateUrl),
        transclude: options.transclude,
        scope: {},
        bindToController: options.bindings || {},
        restrict: options.restrict || 'E'
      };
    }
    ...
    return this.directive(name, factory);
  };

Programmers might build "components" manually with module.directive, in such cases it still works.

If there is a directive with the same name than a component, it means that we have actually two directives with the same name. May be in this case the helper function should throw an error.

If it would change in the future, and components become not implemented through directives, it may broke many codes, like many tests, and them must be fixed.

The cool part is that we can add an alias for free called $directiveController.

[gkalpak]

@drpicox, the "problem" is that multiple directives with the same name is a supported feature (and quite a useful one in several cases). That is the reason why it is not easy to implement this feature for "non-component" directives with predictable behavior.

By "non-component" I mean directives that are not registered using the component() helper. Components (directives registered with the component() helper) are indeed "plain old" directives, but they are guaranteed to have certain attributes (e.g. restrict: 'E', isolate scope, a controller, a controller alias (an a specific default fallback alias) etc). These attributes allow the implementation of $componentController to be predictable in all cases.

[drpicox]

@gkalpak for what I understand you are not telling that the suggested implementation for testing is wrong, but the way to implement the directive is. I explain:

• you say that components are guaranteed to have the following properties: restricted to 'E', isolated (it will change in 1.5.0-rc1), with a controller, a controller alias, ...
• it seems that you imply that you cannot have two components with the same name
• currently with module.component you can change restricted, make non isolated components, make components with replicated names, ...

This code is working in the current implementation (both, 1.5.0-rc0 and current merged for 1.5.0-rc1):

app.component('myComponent', {
  controller: MyControllerAspect1,
});
app.component('myComponent', {
  controller: MyControllerAspect2,
  restrict: 'A',
});

In this case, the current implementation of $componentController for 1.5.0-rc1 is not working properly, because it takes one only of them (the last implemented).

This code is not working in 1.4.8:

app.directive('myDirective', function() { 
  return {
    controller: MyControllerAspect1,
    controllerAs: 'aspect1',
  };
});
app.directive('myDirective', function() { 
  return {
    controller: MyControllerAspect2,
    controllerAs: 'aspect2',
  };
});

In order to avoid multiple controllers for the same component restrict must be forced always to 'E'.

The other point to consider is what happens with decorators. It is legal to do:

app.decorator('myComponentDirective', function($delegate) {
  var directive = $delegate[0];
  directive.controller = MyControllerAspect3;
  return $delegate;
});

In such case, the original solution that I try to amend, in addition to save bytes in the angular.min.js, it also is getting the right controller, a controller that must satisfy the tested interface.

So, there are the following ways to proceed:

1. Modify semantics to force all components restrict to 'E' (which seems reasonable to me),
2. Modify the way of creating components to make angular fail when creating two components with the same name (code into angular.min.js is required),
3. Accept current implementation that can be multiple components with the same name,

How to implement $componentController in each case:

• Case 1: components must be 'E'
  $componentController: Instead of getting the first element from directives it has to look for the directive with no restrict and controller or restrict containing 'E' and controller. It must fail if two directives satisfies this condition (as it would fail the template compilation).
• Case 2: no two components with the same name
  compile.js: add an structure and control to fail if two components with the same name are added
  $componentController: same implementation to deal with module.decorator thing
• Case 3: as currently implemented
  $componentController: add one more argument to request the restrictness of the controller, by default 'E'. Then follow as case 1 but with the selected restrictness.
• Case 4: none of above
  That is the option that I said in previous comments: failing if there are more than one directive with the same name it was intended to be consistent in the testing results. The idea is that you want to put multiple components, directives, whatever over the same name (something not common), is up to you to decide how to test it and you can tweak it as much as you like, so you will probably do not use $componentController.

@petebacondarwin and @gkalpak any suggestion for which case should be applied?

[gkalpak]

@drpicox, indeed I missed that you can have several components with the same name. In that case, $componentController may not bahave as expected.

Maybe it's a good idea to only allow one component per "restrict" type (maybe even only allow E and A). The only usecase that A is useful is for elements that only accept specific type of children (e.g. table cannot have <my-component> as a child, but it can have <tr my-component>).
Still, it it worth considering to only allow restrict: 'E' for component and handle such case using non-component directives.

I think we need to re-think $componentController 😕

[drpicox]

Worse that that, rethink component.
Once component is defined, solve $componentController is just straight forward (I give 4 implementations).

[petebacondarwin]

OK, so we should restrict mod.component only to elements. That should fix most of the problems, since it would not be possible to have a running application which has an element that matches two components.

[petebacondarwin]

Then the $componentController could filter out non-element directives. There should only be one of those. If there was not exactly one, then it should throw an error.

[drpicox]

So Case 1. I'll try to do the changes later today.

[drpicox]

Fixed.
It should be ok now.

[petebacondarwin]

@drpicox I guess you are trying to rebase?

[drpicox]

Yep!
#13742

[drpicox]

Rebase is my pending lesson in git.