Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Recommend against controllerAs vm #462

Closed
wesleycho opened this issue Jul 30, 2015 · 18 comments
Closed

Recommend against controllerAs vm #462

wesleycho opened this issue Jul 30, 2015 · 18 comments

Comments

@wesleycho
Copy link
Contributor

There has been talk over the past few months about the vm pattern - var ctrl = this seems to almost unilaterally be preferred by all of the experts I have talked to (& by myself), but this is minor.

The problematic recommendations are controllerAs: vm and Foo as vm - this is less searchable, as well as obfuscating the name. For example, Auth as auth would allow the user to describe in the template more appropriately, such as <form ng-submit="auth.login()"> - human readability is much better, as well as searchability due to being able to quickly identify the template of interest. Doing grep -r 'auth' src/ is sure to return much more relevant results than grep -r 'vm' src/, and similarly for any other project-wide searches.

@dietergeerts
Copy link

vm stands for ViewModel and imho is a far better name than ctrl, which stands for controller. Yes I know that Angular had chosen the name Controller, but they are used as a ViewModel, hence they are a model for your view. Many people (including experts) have worked more with controllers than with viewmodels, that's why they prefer controller, which I think is wrong. ViewModel is just so much more flexible and self contained.

About the use of as vm, you can choose what you call it. The view only have to know what it will be called when the controller is set through route or ngInclude, but that's no problem as views are always build for certain viewmodel(s). I prefer just vm, as you know it is a viewmodel... Never had problems with searchability, one view is for one viewmodel.

@wesleycho
Copy link
Contributor Author

Searchability is a real problem though - when searching for what template the line is in, you get potentially a huge number of matches.

Almost every developer I have spoken with recommends against this, including maintainers for some of the most used libraries in the Angular ecosystem (as well as myself).

This pattern has also caused a lot of headaches for others in the community when helping developers who are new to Angular, increasing the confusion with terms to grapple for those in need of help.

@robrothedev
Copy link

controllerAs: 'vm' is definitely too generic. Personally I use the same name as my controller so it's extremely explicit what controller the view binds to:

function VideoListCtrl() {
  var vm = this;
  vm.list = [];
}
<div ng-controller="VideoListCtrl as video_list_ctrl">
  <ul>
    <li ng-repeat="video in video_list_ctrl.video_list">{{video.title}}</li>
 </ul>
</div>

As far as var vm = this; that can also be whatever you'd like to call it.

I prefer the controllerAs syntax as it's a much cleaner implementation than throwing $scope around. The nice thing is that the option is there. It's up to the developer to determine which way to go.

@johnpapa
Copy link
Owner

johnpapa commented Aug 2, 2015

In HTML ... It can be whatever you want to call it. I start with vm and I don;t change it unless I get nested controllers and I can't tell which is which. Then I renamed them to customerVm and ordersVm, as an example. But I do not rename them unless I have that issue. Otherwise, when there is just 1 vm, vm more than suffices.

In the JavaScript, I only do vm since it is perfectly obvious I am in the Controller.

closing this ... had this chat a few times :) good conversation tho!

@johnpapa johnpapa closed this as completed Aug 2, 2015
@wesleycho
Copy link
Contributor Author

The problem I have is that as it is currently recommended in the styleguide, it does not make this distinction with the templates and it is showing negative consequences across the help channels in the ecosystem, including productivity loss.

I agree with the controller part in JavaScript (vm or ctrl or whatever, doesn't matter much) - the template part is the problem sector. If this elaboration was codified in the styleguide (to diversify with customerVm or ordersVm as needed), this problem wouldn't come up constantly - many users have ended up confused with what is happening, and this has obfuscated ease of debugging, especially when many work with non-trivial applications.

@BobbieBarker
Copy link

Further complicating matters is that most new comers can't make that distinction between when to just use "vm" and when to have a different name spacing. So, while we're sitting in IRC/Slack helping people we're seeing that virtually all of their controller's are name spaced with "vm". It's not an ideal convention; I think we can do better as a community.

@hassanbazzi
Copy link

+1

1 similar comment
@MelissaMMDP
Copy link
Contributor

+1

@zachlysobey
Copy link
Contributor

I ran into this myself when first adopting this styleguide. It took significant time to go back and clean up a whole mess of vms in my views.

@johnpapa
Copy link
Owner

johnpapa commented Oct 9, 2015

many controllers named vm is not bad. controllers all with super long names that are crystal clear are not bad either. You pick what makes sense. I do not believe in an all or nothing here. I always start with vm. Why? Because most of the time I have 1 controller assigned to the view and its crystal clear. It's a hassle and doesn't clarify anything to have more than vm in the view if I have one controller. When I add multiple controllers and have multiple of these, it makes sense to name them customerVm and orderVm, for example. Pick your favorite.

This guide is not intended to explain Angular. That's what the docs do. There is material that explains controller as. There are blog posts (one which I wrote).

@BobbieBarker "Further complicating matters is that most new comers can't make that distinction between when to just use "vm" and when to have a different name spacing."

They dont have to. You can use vm everywhere if you want. If its confusing, then you use a new name. I think that's a simple rule. Stay simple, til its not simple.

To be clear, I'm only talking about the vm in the view ... and my guidelines are

  1. Use vm
  2. When compliance w/ jslint self invoke function #1 is not clear, use a different name

@wesleycho
Copy link
Contributor Author

The point is that simply using vm in the view without context is precisely what causes confusion - you, yourself admitted that adding more context to vm such as customerVm is better. There are whole teams at companies that take the guide at face value, and not adding that context to vm is what does a disservice. It simply is not legible or maintainable.

My main request is that this needs to be codified into the guide itself.

@MelissaMMDP
Copy link
Contributor

IMO - I am against using vm only because I feel it conflicts with the "Why AngularJS?" section at angularjs.org; specifically the last two sentences shown below.

"AngularJS lets you extend HTML vocabulary for your application. The resulting environment is extraordinarily expressive, readable, and quick to develop."

I feel creating a naming convention from the start that is expressive and readable, e.g., 'customerVm and orderVm, is a best practice. Less chance of finding yourself in the situation @zachlysobey described.

@johnpapa
Copy link
Owner

johnpapa commented Oct 9, 2015

@wesleycho I did not say customerVm is better. I said if vm is not clear to you, make it customerVm.

The guide is a guide. Most folks take it and adjust as needed. Absolutely feel free to adjust and name with long names if you like. Have a PR that shows balance? sure, I'll review it.

I've also seen many apps where people go nuts on names with salesCustomerDetailsVm and employeePricingDetailsVm and orderDetailsVm and massively longer names all over their views. The html ends up being riddled with long names for every binding that make it harder to read. I've maintained these apps too. Sigh. There is a balance.

The guide starts with vm. Where you go from there is absolutely up to you.

@MelissaMMDP
Copy link
Contributor

Makes sense @johnpapa

@wesleycho
Copy link
Contributor Author

This is fallacious reasoning - just because some people abuse naming with lengthy names, that going the opposite extreme is what is needed. I can tell you from having helped and mentored many people in the community, and as well as talking to others who are considered experts in the community as well, the current claim that it is simpler is simply not the case. On the contrary, I have seen near unanimous rejection of this.

For a supposed style guide, this is just flat out wrong not to elaborate here in the guide itself when the guide elaborates pretty much everything else in detail. It is incongruous, has demonstrable side-effects, and frankly, avoids addressing the argument for why to include the advice from the guide itself. This response is not a strong justification as to why not amend the guide.

@johnpapa
Copy link
Owner

johnpapa commented Oct 9, 2015

You are confirming my point. Neither extreme is best. Common sense prevails and help make clarity in our code.

I never said it wont be elaborated. I said make a PR.

I've been parts of quite a few apps. vm has been perfectly fine and clear for the vast majority of those. Your disagreement doesnt make any of this right or wrong. The answer is clear and I have said it repeatedly. Make a PR that follows a balanced approach and I will consider it.

I'll say it again ... make a PR that is balanced and it will be considered, just like the other 100 contributors have done.

This response is not a strong justification as to why not amend the guide.

I'm not saying it won't be changed. I've said the opposite ... make a PR that is balanced and it will be considered.

There's nothing more to say

@wesleycho
Copy link
Contributor Author

I missed the PR bit, I apologize - but I will say that closing the issue prematurely then was not the right move. The closed issue gave me the impression that this wouldn't be considered (currently juggling drama that suddenly spawned in UI Bootstrap in the past couple of hours, so apologies if my wording and clarity are not quite there currently).

@johnpapa
Copy link
Owner

johnpapa commented Oct 9, 2015

everything is considered, as long as it is an angular style and follows the what, why, and how (and how not sometimes). and in this case a when would be good.

drKnoxy added a commit to drKnoxy/angular-styleguide that referenced this issue Nov 25, 2015
Also i think the third bullet point helps cover the fact that you *can* use multiple names
johnpapa#577
johnpapa#462
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants