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

Project style guide? #449

Closed
kingcody opened this issue Aug 15, 2014 · 23 comments
Closed

Project style guide? #449

kingcody opened this issue Aug 15, 2014 · 23 comments
Labels
Milestone

Comments

@kingcody
Copy link
Member

I read an interesting blog post by @toddmotto; it's an opinionated angularjs style guide for teams. He has a git repo that contains his guide and will be the place for any future updates. He definitely makes some good points and I would recommend the read.

It kinda got me thinking. Seeing as generator-angular-fullstack is a generator with best practices in mind, I was wondering if we should look at adopting/formulating a project style guide. I feel that it could improve the quality and cohesiveness of the project's code base.

I wouldn't go so far as not accepting PR's that don't comply with the guide; but merely suggest that they follow. Much like the current commit message suggestion.

Just my thoughts.

I'd like to hear what others think.

@DaftMonk
Copy link
Member

👍 Some excellent suggestions in there.

@JaKXz JaKXz added the info label Aug 15, 2014
@JaKXz
Copy link
Collaborator

JaKXz commented Aug 15, 2014

It would be cool to get some of those things into the editorConfig, for example.

@JaKXz
Copy link
Collaborator

JaKXz commented Aug 15, 2014

Quick read reminds me of #263 which may present a problem.

@remicastaing
Copy link
Contributor

Her another angular style guide that got some attention the last days: https://github.com/johnpapa/angularjs-styleguide.

@kingcody
Copy link
Member Author

@JaKXz to be honest I'm not that experienced with coffeescript, but I put a small fiddle together using coffee and the new controller as syntax.

http://jsfiddle.net/gvJdn/179/

I know support for 'controller as' was spotty in earlier versions of angular. Perhaps I'll get some time this weekend to take a closer look, in particular using fullstack v2. See if #263 still seems to be present.

@kingcody
Copy link
Member Author

@remicastaing awesome, nice find.

@kingcody
Copy link
Member Author

Another thought I'd like to add to this is, maybe the use of grunt-jsbeautifier would be useful for testing. It could be configured like:

"jsbeautifier" : {
    "default": {
        src : ["**/*.js"]
    },
    "verify": {
        src : ["**/*.js"],
        options : {
            mode:"VERIFY_ONLY"
        }
    }
}

I'm guessing that a grunt task could be configured to run for the different variations that are tested.

Could help with PRs, commits, etc...

@kingcody
Copy link
Member Author

Obviously an appropriate jsbeautifier config would be needed for verifying the generated code.

@toddmotto
Copy link

Thanks for the mention guys, good luck with the project :)

John and I collaborated on our styleguides, the reason they're independent is we have differences in some things and wanted a way to compare.

If there's anything worth integrating into the styleguide (things that may be missing/worth a mention from this generator) then do drop an issue/PR! :)

Happy coding!

@DaftMonk DaftMonk added this to the 2.1.0 milestone Aug 18, 2014
@DaftMonk
Copy link
Member

We should also look at adopting a styleguide for the node side of things to keep it consistent.

@kingcody
Copy link
Member Author

I absolutely agree.

side note, @DaftMonk so in trying to learn some testing frameworks I've been looking at extending/completing some of the test for the project. Would that be a welcome addition?

@DaftMonk
Copy link
Member

@kingcody We could definitely use some more tests.

@kingcody
Copy link
Member Author

I'm working with chai as promised with mocha atm. Really simplifies the model tests since mocha accepts promises.

@DaftMonk
Copy link
Member

Mocha + Chai is fantastic. Gives you a ton of flexiblity when writing tests.

@kingcody
Copy link
Member Author

Wondering if node-jscs might be a good tool for this, in particular the grunt task.

@remicastaing
Copy link
Contributor

And here another JS quality guide that also got some attention in the github community.

@kingcody
Copy link
Member Author

kingcody commented Oct 1, 2014

Closed by 8a1a245

@kingcody kingcody closed this as completed Oct 1, 2014
@andrewstuart
Copy link
Member

I hope it's not too late for me to throw in my two cents regarding style, since a few style guides have been linked. They do have some great stuff, and aggregate a lot of information scattered throughout a lot of the official meetup videos on youtube.

I will say, though that I disagree with their controller approach. They recommend avoiding the angular.module('foo').controller('bar', function bar() { .... }) syntax for cleanness and to keep a 'dot in the scope' but then go on to say you should use IIFEs to stop from polluting the global scope and then use var vm = this; inside every controller instead of $scope.

The $parent problem mentioned is a symptom of a deeper problem of not using services and objects to organize your data and business logic (something I think we should recommend), and I don't think it should be resolved by throwing away Angular's awesome $scope inheritance. As an aside, this inside the controller constructor is definitely not bound to $scope as claimed. So you don't get any sort of inheritance. If you were doing things properly from the beginning, organizing data into objects, foo.bar becomes baz.foo.bar. If you've read much of the Angular code, you know how integral $scope is to everything. For a small example, try to access ng-repeated elements without using the new $scope created.

I hope I'm not coming across as hostile, because I did like a lot of what the style guides had. Heck, I learned a few things reading them. It's just my strong opinion that their controllerAs (and directive controllerAs) recommendation makes things less readable, creates more typing, and throws away the best parts of $scope.

@kingcody
Copy link
Member Author

kingcody commented Oct 2, 2014

I hope it's not too late for me to throw in my two cents.

Not at all :)

I think I gathered what you were saying but correct me if I'm wrong...

I agree with several of your observations. For one the definition syntax, I don't believe IIFE is necessary especially when defining everything within the angular namespace. As for data organization I prefer to use factories and services and remove as much from my controller as possible. From what I've always heard from the angular team is that controllers should be very lite and be used to simply wire up services and data sources to the DOM/scope.

It's my practice to use the controllerAs syntax and bind controller specific data and services to the object returned from the controller (I prefer to return a controller obj instead of binding to this). I then bind more general methods such as Auth.X to the scope.

My reasoning is:
By using ctrlAs you can do things like this Navbar.links in the DOM which I think is more descriptive and can be referenced elsewhere in the DOM with closure. Also it's less likely to cause naming conflicts versus strictly using the scope. However for thing more general, that are likely to be used in the same way everywhere, I prefer those on the scope for maximum reusability.

TL;DR
Global(ish) data and services on scope, specific data and services on the controller object.

Anyone else have experience or opinions on controllerAs syntax?

@andrewstuart
Copy link
Member

Hmm. I guess my view <my-view> has just been that the controller is there to bind data to the scope, not to have data itself and be on the scope as another property. I think the instantiation has less to do with it being a class and more to do with dependency injection and cleanup/gc. If I want to share data, my first thought is "service!" and then anything that wants that data can ask for the service to be injected. I've looked a lot today at the controllerAs advocates and I guess most of the patterns they advocate just feel like gymnastics to get back what $scope was designed for in the first place. Like trying to do a for loop in SQL to filter out data instead of WHERE. </my-view>

Definitely agree on using services and factories to house data, though. I'm also interested in what other people have to say. I guess it probably would be something to leave out of the generator either way, since the ngController directive is only really used for the navbar.

Heads up on returning an object in your controller, by the way. If I read it right, this issue may mean trouble for that.

@kingcody
Copy link
Member Author

kingcody commented Oct 2, 2014

Heads up on returning an object in your controller, by the way. If I read it right

I would believe that you did indeed read it correctly, and thank you for pointing that out to me; will definitely save me some future headaches.

@andrewstuart I'm actually glad that you have a different perception than I. Would you perhaps explain what you see would be lost, as far as features, by attaching specific data to the controller instance? I'd like to expand my own view point 😄

@andrewstuart
Copy link
Member

I'm glad as well. It definitely makes me think more about the reasons I recommend using $scope and not controllerAs.

The only thing, feature wise, that I can think of that you lose is the ability to $scope.$watch on a string expression and only use a single watch function for all watchers on that same parsed expression..

Really, not much of a functional loss there, IMO. My main concerns are more stylistic and semantic. I don't care the semantic duality of a controller being a storage container or class. Rather, I prefer to use my controller as an orchestrator of services, including $scope, controlling the way they interact. Typically if I hear somebody saying to avoid $scope or $scope.$watch entirely, I get skeptical, since that's the bread and butter of Angular and behind every {{binding}}, not to mention instantiated every time you add a controller whether you inject it or not. Sure, you can basically reimplement $watch in all your services, or use properties or functions to get/set so that you can react to changes, but $watch is so much simpler, optimized (if you use string expressions), and it's already implemented and pretty ubiquitous.

To get a named property on the scope, I'd just personally write a scopeAs directive that requires ngController and simply does scope[attrs.scopeAs] = scope If I really really wanted to that hint in the template when I'm working on the presentation.

At the end of the day, my recommendation in a style guide would probably be that they're both valid styles but that it's strongly recommended you pick only one style for your project. Maybe even make a generator option (asked at the app portion, not for every controller) so that both styles can be generated based on project preferences. Like I said, I'm definitely glad you got me thinking more about why I felt that way. Helps me keep an open mind.

@remicastaing
Copy link
Contributor

Yet another style guide: https://github.com/gocardless/angularjs-style-guide

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

No branches or pull requests

7 participants