Skip to content

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

Closed
wants to merge 1 commit into from

Conversation

jvandemo
Copy link

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 using ng-controller:

<div ng-controller="Avengers as vm">
</div>

This is not a best practice and should ideally be replaced with:

<div ng-controller="AvengersController as avengers">
</div>

to allow nesting of controllers.

The new syntax is also conform to Angular 2's default setting.

@kmaida
Copy link

kmaida commented May 13, 2015

+1

@reflexdemon
Copy link
Contributor

Is this something related to #309?

@wardbell
Copy link
Contributor

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

  1. To the name of the controller (you've added the 'Controller' particle where we tend to omit it)
  2. To the name of the variable within the template (e.g., from 'vm' to 'avengers').

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 vm and moving on with our lives.

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.

Btw, I'm not seeing this as an "Ng 2 default setting" anyway as it has a quite different style of template construction. We're talking Ng 1.x here, right? It's worth observing that the Ng team itself has not asked John to change his recommendation.

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.

I don't want to bloat the Style Guide either. I'd settle for a summary explanation and a link to a the elaborated discussion. Perhaps a link to this repository's issues.

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.

@johnpapa
Copy link
Owner

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.

@johnpapa johnpapa closed this May 13, 2015
@johnpapa
Copy link
Owner

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.

@wardbell
Copy link
Contributor

The style guide does say "Controller" suffix, as per discussions with Igor

So hard to keep up :-)

@jvandemo
Copy link
Author

@johnpapa — A section for nested controllers makes sense. Will add a PR for this.

@wardbell — Thanks for the tips! Will use them in the upcoming PR. 👍

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

Successfully merging this pull request may close these issues.

5 participants