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

REQUEST FOR FEEDBACK: angular.component() - default directive controller name #13664

Closed
petebacondarwin opened this issue Jan 2, 2016 · 59 comments

Comments

@petebacondarwin
Copy link
Contributor

We should use a consistent default value for the name of a component's directive controller when it is attached to the scope. See #10007 (comment)

Currently we are defaulting to the canonical name of the component. This is not ideal as

a) component names can become long and unwieldy for use in a template
b) it is more complicated to automatically update the template to be used in Angular 2, where the context is the controller.

The criteria for the name are:

  1. it must be the same for all components
  2. it must start with $
  3. it must be short (2-4 chars)

In addition the name should represent what is actually being published to the scope.

Some of previous suggestions include:

  • vm - this is the commonly used name in many applications but the controller is not necessarily a "view model"
  • $comp - this is the current suggestion from the team but can be confused with compare and is not that short
  • $ctrl - this can be confused with input ConTRoL elements
  • $this - the controller is not really this in the template, since the context is still actually the scope
@drpicox
Copy link
Contributor

drpicox commented Jan 2, 2016

c) programmers are tempted to use isolate:false and access to ancestors controllers directly.

@petebacondarwin
Copy link
Contributor Author

@drpicox - I am tempted to say that we ban isolate: false for components created using this helper.

@drpicox
Copy link
Contributor

drpicox commented Jan 3, 2016

I agree, after considering this:

  • isolate: true when restrict is 'E': for me they are truly components, in where $ctrl notation has full meaning
  • isolate: false when restrict is 'A': for me they are decorators, a kind of enhancer of the existing components, in this case $ctrl collides so the current nomenclature it's fine

But I consider that the second case is better to do with directives, decorators are not frequent, usually low level, and not suited for juniors.

So probably is good idea to ban isolate: false.

In other track of thinking, about decorators, a function to get the controller of the 'E' component of the current entity should be good idea, specially to write generic decorators to deal with any current component (require forces to know in advance which controller is and you cannot use a kind of interface of what are you looking for).

@Narretz Narretz added this to the 1.5.x - migration-facilitation milestone Jan 4, 2016
@Narretz
Copy link
Contributor

Narretz commented Jan 4, 2016

I like $cmp, but find $comp even better, because it's clearer.

@mgol
Copy link
Member

mgol commented Jan 4, 2016

I like $cmp, but find $comp even better, because it's clearer.

I like $comp. Whenever I see $cmp I think "compare".

@tabanliviu
Copy link

different suggestion: why not have the name of the component as the controller instance name?
ex: user-profile -> scope.userProfile

@mgol
Copy link
Member

mgol commented Jan 4, 2016

@tabanliviu That's how it's currently done on master but @petebacondarwin mentioned in this very issue, in the first post, why a common name is better.

@tabanliviu
Copy link

@mgol 👍 I think I glossed over that when I read the ticket. Should this change be considered more of a ngUpgrade issue? I think in the context of angular 1.x the current implementation is a good solution. Perhaps exposing this as a settings function that takes the component name and outputs the controller name? this way it serves the current pattern and a future migration path.

@vivainio
Copy link

vivainio commented Jan 4, 2016

Bikeshed time.

+1 for $ctrl.

Ctrl has lots of pre-existing culture in Angular 1 documentation and examples as the "controller" suffix. Try googling "angular ctrl" for good measure.

The current choice of deriving it from component name is quite nasty, as they indeed tend to get pretty long in real applications.

@petebacondarwin petebacondarwin modified the milestones: 1.5.0-rc.1, 1.5.x - migration-facilitation Jan 4, 2016
@petebacondarwin petebacondarwin changed the title angular.component - default directive controller name REQUEST FOR FEEDBACK: angular.component() - default directive controller name Jan 4, 2016
@jtrussell
Copy link

👍 for $ctrl, $vm as a default feels more like a statement of how controllers should be used.

@MikeRyanDev
Copy link
Member

+1 for $ctrl.

We debated this pretty heavily on the ng-forward team and decided that ctrl was a less loaded term than vm.

@0x-r4bbit
Copy link
Contributor

I vote for $ctrl and I'd be very happy if this encourages ppl to not call their controllers vm anymore :P

Oh, I'd also like to add that

this can be confused with input ConTRoL elements

Nah. Not really.

@juristr
Copy link

juristr commented Jan 4, 2016

$ctrl

I think that would be most comprehensible and intuitive for everyone.

@orizens
Copy link

orizens commented Jan 4, 2016

+1 $ctrl

Other suggestions:

  1. $as - like controller as
  2. $at - like @ - while in coffee script it references to 'this' context
  3. $class - although 5 chars, it's close to ng2 component class notation.
  4. $prox - since conceptually, the Ctrl instance is a proxy to a services layer
  5. $ctx - shortcut for context
    Thanks.

@e-oz
Copy link

e-oz commented Jan 4, 2016

I vote for $this because in ng2 this of controller is context of template (and In my opinion component is very good in role of transition tool between ng1 and ng2).

@chrisse27
Copy link

+1 for $ctrl

@deleonio
Copy link

deleonio commented Jan 4, 2016

I also prefer the $ctrl property, because it represent the component controller.

+1 for $ctrl

@shahata
Copy link
Contributor

shahata commented Jan 4, 2016

+1 for $this

I would even go with this and drop the $ if it wasn't too much of a hack to do it. It is also the only option which is not short for something else, and I hate abbreviations. :)

@maartentibau
Copy link

I would go for $ctrl or $cc (being short for component controller)

@seefeld
Copy link

seefeld commented Jan 5, 2016

+1 for $ctrl

@Puigcerber
Copy link
Contributor

We could call it just $troll, it has a bit of $this and half of $controller. No, just kidding, I'm fine with $ctrl. 👍

@johnpapa
Copy link
Contributor

johnpapa commented Jan 6, 2016

I am not a fan of confusing names. I think ctrl is confusing. It is controller? control (like html control)? and isnt this for a Component?

I vote for either vm or comp. vm is commonly used and easy to explain. comp is new, but not hard to divine.

@petebacondarwin
Copy link
Contributor Author

How about $ctlr (i.e. ConTroLleR) rather than $ctrl?

@tabanliviu
Copy link

+1 $comp

@shairez
Copy link
Contributor

shairez commented Jan 6, 2016

@petebacondarwin Oh the amount of dyslectics (like myself) who will bomb us with issues about this... :)

@drpicox Thanks for the explanation, I see your points and they are valid. it's a tough one, but I can share that at least from my experience, I had no trouble teaching developers the "vm" convention, and helped a few companies structure their massive app that way, they got it pretty fast, but maybe I'm alone in this experience.

But I understand your points. agree with $

I'm still for $vm though, but am fine with $comp as well...

@dmitriz
Copy link
Contributor

dmitriz commented Jan 7, 2016

@wesleycho from the Angular UI Bootstrap team seems to be strongly against vm:
#10007 (comment)

@sebald
Copy link

sebald commented Jan 7, 2016

+1 for $ctrl

@drpicox
Copy link
Contributor

drpicox commented Jan 7, 2016

@shairez I share completely your point about having a convention, I'm a Freelance Architect with tens of projects behind, the vm convention helped a lot, but I have still some issues. It turns out that there are people resisting to use it. Probably the resistance should be lower if this convention comes from Angular itself, but I am sure that if the name was $ctrl they would accept it as is, without any resistance. $ctrl is just straight forward.

@petebacondarwin
Copy link
Contributor Author

So the votes are in and it looks like this:

$comp  4
$cmp   2
$ctrl  19
$vm    3
$this  3
$ctx   2
$vc    1

The clear favourite is $ctrl. As well as being popular it passes the criteria posted at the top of this issue. In addition it doesn't introduce any particularly new concept. The thing being referred to really is a controller (a component/directive controller) of which Angular developers already understand and just as some developers have gotten used to using vm in their directives it will not take long for developers to cotton on to this default.

@shahata shahata closed this as completed in d91cf16 Jan 8, 2016
@shairez
Copy link
Contributor

shairez commented Jan 11, 2016

Awesome, $ctrl it is!

Issue for 1.4 and lower - can't name an 'as name' with $ctrl

Another concern I'd like to raise is that in angular 1.4 and lower, we can't really use "as names" starting with a $ sign.

It gives the following error:
Error: [$controller:ctrlfmt] Badly formed controller string

Some companies have trouble upgrading to the latest versions, and it can take them a several months.

They still want to keep up with the conventions, so their upgrade process will be simpler in the future.

For them, switching from vm to $ctrl is impossible.

What do you think? any suggestions?

@orizens
Copy link

orizens commented Jan 11, 2016

perhaps migrate in phases:
start with converting vm to ctrl
when 1.5 is release, "upgrade" ctrl to $ctrl

Another possible way - although verbose - is to generate controllerAs alias in runtime, checking angular.version. something like:

 angular
        .module('github')
        .directive('issueThread', issueThread);

    /* @ngInject */
    function issueThread () {
        // this can be required as a module if using some module loader
        // or - another way is using global on angular namespace (i know it a bad practice - hwoever just to indicate reuse of this check 
        let prefix = angular.version.minor === 5 ? '$' : '';
        let controllerAs = prefix + 'ctrl';
        // with template strings
        var controllerAs = `${prefix}ctrl`;

        var directive = {
            controller: controller,
            restrict: 'E'
        };
        return directive;

        function controller() {
        }
    }

@drpicox
Copy link
Contributor

drpicox commented Jan 11, 2016

@orizens What about templates?

@shairez Uhmmm it makes sense, $ symbols are intended only for angular internals... may be having some kind of forward compatibility in the next minor is nice.

@orizens
Copy link

orizens commented Jan 11, 2016

@drpicox you got a point there :).
Again, one solution i can think of (hacky one...), is, "replace" ctrl with $ctrl in template in runtime / build. That can be achieved easily if the project is built with es6 and modules. otherwise, it's a task for gulp/grunt/npm at build time.

@gkalpak
Copy link
Member

gkalpak commented Jan 11, 2016

Why not just use controllerAs ?
It's not an ideal solution (and indeed we might need to revise the RegExp that extracts the identifier from the controller string (if any), but using controllerAs is both backwards and forwards compatible :)

@gkalpak
Copy link
Member

gkalpak commented Jan 11, 2016

(If anyone wants to have a shot at updating that indentifier extracting RegExp, it's right there btw.)

@shairez
Copy link
Contributor

shairez commented Jan 11, 2016

@gkalpak that's a good point, moving forward promoting the use of the controllerAs property is good as more and more people will also transition to using components in their 1.4 and lower versions I believe.

But, I think it might be confusing if we'll start teaching people about $ctrl and in some cases it works and in some it isn't.

So a forward compatibility (not sure how), is a great idea!

@petebacondarwin
Copy link
Contributor Author

@shairez did you (can you) create a new issue to track this?

gkalpak added a commit to gkalpak/angular.js that referenced this issue Jan 11, 2016
gkalpak added a commit to gkalpak/angular.js that referenced this issue Jan 11, 2016
@gkalpak
Copy link
Member

gkalpak commented Jan 11, 2016

I created #13736 which allows $ in identifier, when using <ctrl> as <identifier>.
Still the allowed identifiers are different between controller: '... as ...' vs controllerAs: '...'.

That said, I am not sure that promoting controller: '... as $ctrl' is a good way of keeping up with conventions. It is much more difficult to upgrade controller: '... as $ctrl' than controller: '...', controllerAs: '$ctrl'.

@petebacondarwin
Copy link
Contributor Author

Thanks @gkalpak - I agree that we should probably encourage use of the controllerAs property rather than the controller as syntax.

@frfancha
Copy link

One thing is: documentation of component says: "Component definitions are very simple and do not require much of the complexity behind defining general directives".
Another one: controller in directive is only necessary if you build complex directives talking to each other. Otherwise a link function is more than enough (e.g. "use controller when you want to expose an API to other directives. Otherwise use link" in Developer Guide directive, and from my experience directives implementing same functionality with link instead of controllers used some hundred times in ng-repeat are much quicker.
So...
I don't find in Component ("simple" directive) the way to do "simple" (link function), only the heavy one (controller).
Do I miss something?
Thanks for the explanation.

@petebacondarwin
Copy link
Contributor Author

@frfancha - the performance improvement is due to not having to use the $injector to instantiate the controller, right? Perhaps you have some performance measurements you can provide?

The idea of the component helper is to make it simpler (in LOC sense) to write component type (isolate, element) directives; and easier to write code that is more inline with how things are done in Angular 2.

If there is a performance issue in a specific app then it would be fairly straightforward to convert a component directive over to use the more general directive helper.

I think we need to take a look at the other API docs and developer guides to ensure they are consistent with the new component() helper.

petebacondarwin pushed a commit that referenced this issue Jan 11, 2016
@frfancha
Copy link

@petebacondarwin At the beginning we wrote all our directives with controller, just because it was shown this way in the first tutorial we followed.
Only after we found it took about 15 seconds to "open" a certain page with 1000 directives (5 by "rows" in ng-repeat x 200), we read more about directives and understood that controllers are useless if you don't "speak" between directives (by requiring the controller of another one). After "rewriting" all with link functions instead of controllers (rewrite is a big word as it was just copy/paste of the code in link instead of controller), time to display the page was 8 seconds.
Note that these are firefox measures, at that time we didn't use Chrome. Now we use it and I estimate the time in Chrome to be the third of Firefox (and memory usage the fourth (and without memory leak, which is great, in Firefox our application is known to be "slow in the afternoon)).
We are generally very happy with angular (we have converted all our data-entry applications from Smalltalk windows application to WEB API + angular (in case that would interest you to see it?? I'm sometimes in London).
But I'm surprised by the choice of controller to support the "simple way" to do directive

@shairez
Copy link
Contributor

shairez commented Jan 12, 2016

thanks @gkalpak !

@petebacondarwin a separate issue isn't relevant anymore right? (Because of the PR)

I agree (as I wrote), we should educate people to use the controllerAs property, but just mentioned it because I predict people will run into it.

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

No branches or pull requests