Implement AngularJS style guide

[cjcenizal]

Goal

Pick/define an AngularJS style guide:

• John Papa styleguide
• Todd Motto styleguide - Angular 1.4.x (more applicable to Kibana)
• Todd Motto styleguide - Angular 1.x (less applicable to Kibana)

General thoughts

1. We could take a horizontal approach of implementing the style guide. E.g. update all services, then update all directives. We could explore ways of automating this.
2. We could also take a vertical approach. This would consist of identifying discrete systems in our codebase which need refactoring/redesigning, and then applying the style guide to the new/refactored code. This overlaps a bit with Architectural problems and anti-patterns in our code. #9050.
3. We can create automated checks to enforce as many rules in the style guide as possible.
4. We can build a CLI tool, e.g. Yeoman generator or something like Angular-CI, which helps us scaffold files which follow some of these patterns and encourage consistency.

[w33ble]

I've been reading through the John Papa Styleguide, and so far I really like what I see. I think the stuff he recommends in there will help aid comprehension of our directives considerably. Specifically I'm thinking about the "Bindings up top" and the "LIFT" recommendations. The controllerAs stuff also looks promising, but that's not a syntax I know well, so I need to read up on Angular a bit.

That guide also references to Todd Motto's Styleguide and mentioned some subtle differences with it, and now that the meeting was pushed again, I plan to read through that next.

[cjcenizal]

I've read through the linked style guides. Here are my thoughts:

• The Todd Motto 1.x styleguide seems largely inapplicable to us, since it's geared towards 1.5 with an eye towards migrating to 2.0. We're not using 1.5 and I don't think optimizing our codebase for migration to 2.0 is a priority that we've discussed.
• The Todd Motto 1.4.x styleguide seems to be a subset of John Papa's, so I focused on that one more.
• John Papa's styleguide seems great. I think we could benefit from all of his suggestions except a couple, below.
• "Defer Controller Logic to Services" - Good idea, but we do we want/have to use $http? Why couple this kind of logic with Angular?
• "Route Resolve Promises" - A view may want to control how it loads content, and how it presents loading states. This would mean moving this kind of logic out of the route resolver and into the view's controller.
• "Manually Identify Dependencies" - Good idea but I'm not sure about the format. How about stacking them vertically, each dependency on its own line? Then we can use F5 to sort them alphabetically.
• "Folders-by-Feature Structure" - This is definitely better than grouping files by type (e.g. directives, templates, controllers), but it may not be a good fit for our codebase. We should figure out what our needs are first.

[w33ble]

I didn't read "Folders-by-Feature Structure" as a really strict guideline, more about trying to keep to the LIFT principle mentioned in the styleguide (which I love). I think the important part is that we don't try to break up files by type, but instead group things in a sensible way when needed, which we more or less already do.

[spalger]

Here's a summary of my thoughts:

• Todd Motto styleguide - Angular 1.x
  • I agree that this guide focuses on the transition to angular 2 a bit more than necessary, but I like much of its message
    • uses angular 1.5 components (I'd love to upgrade so we can start moving to these)
    • Uses class for controllers, services, etc.
    • Combines import statements and angular modules by exporting the module name
    • Promotes one-way data flow
• Todd Motto styleguide - Angular 1.4.x (more applicable to Kibana)
  • I like how short this guide is
  • my issues with it are:
    • "Pass functions into module methods rather than assign as a callback"
      • I find it confusing to have the provider defined outside of the call to angular.controller/service/directive/etc()
      • I think this disrupts the flow of how an angular module should read:
        1. define the angular module
        2. define a controller
        3. done.
      • becomes:
        1. define a function that has a Ctrl suffix, indicating it's probably a controller
        2. define an angular module
        3. register the Ctrl function as a controller in angular
      • or even worse, when it's defined below
        1. define an angular module
        2. register a function as a controller
        3. hunt for the definition of that controller
    • Services would make better event busses than $rootScope
    • I think we should suggest using $scope.$listen(emitter, event, handler) rather than saying "remember to unbind your event handlers"
    • Mention of $scope.$digest should always be prefixed with "you should never need to use this"
• John Papa styleguide
  • This thing is huge, I'm not going to address the testing, build tools, animation, or editor integration sections
  • My issues with it:
    • Modules
      • Our ui/modules wrapper should probably be removed, but moving away from it at this point is going to be a huge migration. I'm not sold that there is benefit to doing it right now.
    • controllerAs with vm
      • Since we are using es6 and have arrow functions there is no reason to alias this, and I think we should completely discourage it
    • Accessible Members Up Top & Functional Delarations to Hide Implementation Details
      • I really dislike this pattern for a few reasons:
        • Unnecessary implementation details are less common than important ones.
        • When a function follows the "small functions" guideline, it shouldn't get in the way of skimming a module.
        • Listing a series of properties that are assigned to values is not helpful unless you know something about the values
          • in the example, I'd love to know how anyone could make use of vm.refresh without knowing it's a function and what arguments it takes
          • Since I would need to know something about these properties in order to use them I would have to hunt for the definition of the value assigned to the property in a functions soup at the bottom of the file, then head back up to the "bindable members" section to find the next function I would have to hunt for
      • A class definition with a couple methods doesn't have these issues, is a well-understood pattern, and editors support code-folding so people can hide the function body and still see the important parts (that it's a function and takes x,y,z arguments)
    • Defer Controller Logic to Services
      • I agree with @cjcenizal, this is a good idea but I'd love to see if we can make these services not depend on angular
    • Singletons
      • The rule says "Since these are so similar to factories, use a factory instead for consistency.". I would apply the same reasoning but come to the other conclusion. Use Services and not factories.
    • Factories
      • I dislike this definition style for the same reasons listed above. I think a service should just be a class, and that class should have methods, properties, and anything that shouldn't be used outside the class should be prefixed with an underscore.
    • Manually Annotating for Dependency Injection
      • If we really want to uglify our code, we can use ng-annotate.
      • Maintaining those arrays is an unnecessary pain, lets make the computers do it
      • If we want to remove some of the angular dependency injection magic I suggest we use $injector.get('depName'), which seems even more clear than the array and doesn't require maintaining two disconnected by totally dependent lists of values. It also works with "unused var" linter checks.
    • LIFT:
      • L and I in this acronym are totally subjective, which I think disqualifies it from the styleguide. If there is an actual rule that will have the result of helping people "instantly know what [a file] contains and represents", or "Make locating your code intuitive, simple and fast" I'd consider it, but I don't think one exists and this style guide doesn't seem to have any.
      • F and T could be considered for the general styleguide, but I think they are out of scope in regard to angular.
    • Modularity
      • This feels like another general styleguide issue (especially since we don't really use angular modules) not an angular specific one.

[cjcenizal]

Here's a spreadsheet to encapsulate our latest thoughts: https://docs.google.com/spreadsheets/d/1YBZoP133InkoWlAaLQoaE6Fjc5XpfC0sji5W8d8OUt4/edit?usp=sharing

[cjcenizal]

Should we close this?

[w33ble]

We closed the PR, seems fitting we close the issue too.