-
-
Notifications
You must be signed in to change notification settings - Fork 4.1k
Remove "as vm" best practice for ng-controller #401
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
Conversation
+1 |
Is this something related to #309? |
Not so fast! :-) We've been doing fine with "as vm" for a long time. Why should the Style Guide suggest otherwise? What, pray-tell, is your actual recommendation for the alternative. In your example, you're actually making TWO changes
These are different style points and deserve separate treatment. You're also not specific about the guidance for the variable name. Should the 'Controller' particle always be stripped? Can the variable name ever be abbreviated (as I've seen done in other examples)? What is my obligation when the name of the controller changes? I'd like to see more than the simple declaration that it is "not a best practice". Where are your reasons? What are the consequence (pro/con)? Where are the citations? No, just saying "to allow nesting of controllers" is not a reason. You have to show what you mean by nesting controllers and how it interferes with that approach. Then you have to show that "nesting controllers" is itself a common and worthy practice. And then you have to show that using a different name for every VM isn't the PITA (especially as the VM controller names change) that most of us have found it to be relative to just calling it No, "to conform to Angular 2's default settings" is neither a sufficient reason nor a citation. Show us where the Ng team (or anyone) has discussed the merits of one approach versus the other. My quick web search didn't turn up anything. How can this alternative be a "best practice" without supporting evidence? Doing what the Ng team happens to be doing now in Ng 2 (where?) is not a particularly good argument.
A big reason I'd be reluctant to accept a PR such as this ... is that it doesn't communicate ... and a Style Guide is about communication. I think I know what you problems you're trying to solve. But that's not the point. A PR should include language and reasoning that communicates to the reader the "why" as well as the "how". You're making a change to a previous style; the burden of explanation is even higher. At least some mention of pros/cons is in order. This PR, on the other hand, simply punches in a few words. I don't feel that is sufficient.
Finally, just to be clear that I'm not ducking the issue, you can see that I puzzled through this myself in #309 . I came to no conclusion other than that we should watch it. We need more experience and clearer guidance before we make a change. And when we make a change, that change should convey some wisdom in addition to laying down a "rule". I appreciate your best intentions. Let's execute a little better before proceeding. Thanks. |
The style guide does say "Controller" suffix, as per discussions with Igor. Your preference to use "avengers" instead o "vm" is fine, but it's not always better or clear cut. Its a style. By default vm is fine most of the time in my experience, and very clear. if you need more context, by all means, rename it. But it's not going to be the base case for the style guide. Thanks for the feedback. |
I'll add that if you want to extend the style guide by showing how one would solve the problem of nested controller scopes in the HTML, that is an idea I'm all behind. Please feel free to revise and submit. It would need a WHAT, WHY, and HOW. |
So hard to keep up :-) |
This PR updates the parts of the styleguide where
as vm
is promoted.Context
Currently the styleguide recommends to use
as vm
for controllers that are loaded usingng-controller
:This is not a best practice and should ideally be replaced with:
to allow nesting of controllers.
The new syntax is also conform to Angular 2's default setting.