johnpapa / angular-styleguide

Angular Style Guide: A starting point for Angular development teams to provide consistency through good practices.
http://johnpapa.net
MIT License
23.84k stars 4.15k forks source link

ES6 Style Guide Discussion #390

Open philmerrell opened 9 years ago

philmerrell commented 9 years ago

Last week I listened to episode 39 of Adventures in Angular, which was a great discussion around ES6 language features and the tooling to support it.

I've seen some posts on the web about using Angular 1.x in conjunction with ES6, but it's pretty sparse. I'm hoping to strike up a conversation about how we can take all the great ideas in this style guide and apply them in the context of ES6. If possible maybe an ES6 branch of this style guide could be created to help facilitate the discussion with input from the community.

After spending the week in ES6 and angular 1.x, one the most obvious pattern changes is through the use of ES6 modules. I really like how angular modules can be isolated to just an angular module definition file and everything else (controllers, services, directives, etc...) can be plain old JS and imported in using the ES6 module loader.

// app.module.js
import AppCtrl from './app.controller';
import './survey/survey.module';
import './core/core.module';
import './services/services.module';
import 'ui-router';
import 'angular-material';

angular
  .module('app', ['app.core', 'app.services', 'app.survey'])
  .controller('AppCtrl', AppCtrl)
  .config(function() {

  });
/// survey.module.js
'use strict';

import SurveyCtrl from './survey.controller';
import MultiChoiceCtrl from './multi-choice/multi-choice.controller';
import MultiSelectCtrl from './multi-select/multi-select.controller';
import StaggeredMultiSelectCtrl from './staggered-multi-select/staggered-multi-select.controller';

angular
  .module('app.survey', [])
  .controller('SurveyCtrl', SurveyCtrl)
  .controller('MultiChoiceCtrl', MultiChoiceCtrl)
  .controller('MultiSelectCtrl', MultiSelectCtrl)
  .controller('StaggeredMultiSelectCtrl', StaggeredMultiSelectCtrl)
;

I'd be interested to hear thoughts on this approach. In Angular 1.x we still have to inform the Angular module system about controllers, services, etc. With this approach I like that it delegates the wiring up of Angular assets only in the module definition files. I think this could also lend itself well to migrating to Angular 2. Since angular.module will be going away it might be prudent to reduce the amount of angular.module references throughout the code base.

jimlei commented 9 years ago

ES6 is already panning out to be fantastic, can't wait!

The scheme you've drawn up has one feature that really bugs me, and that is if I want to add a controller to the survey module I would have to create a file for it with the implementation and then "map it" in another file. With the current style we don't have to as the controller and the association to the module exist in the same file.

philmerrell commented 9 years ago

@jimlei Thanks for taking a look.

It's absolutely true that with this pattern you would need to map controllers, services, etc. back to a an Angular module file. The thought process was to reduce the footprint of angular.module references in ES6 projects, and to the extent possible keep only what needs to be tied to angular.module in isolated files. That way we can keep everything else agnostic of Angular.

KamBha commented 9 years ago

We have been playing around with ES6 at my workplace. I found the usage of imports to really get in the way as stated by @philmerrell .

That said, the use of let/const, and even classes (when used for defining data objects used in directive), are useful.

Not sure about fat arrows yet. I like the problems that fat arrows fix, but not sure when we will need it.

johnpapa commented 9 years ago

I've gone both ways with this where the module file imports all the things it needs and registers them, as @philmerrell showed. Then the other way of having each file use the module registration, like what I do in ng 1.x/es5. Both work but I feel the former is cleaner for my tastes so far ... but I reserve the right to flip flop :)

I'm tinkering with some ways to write ES6 and TypeScript and use modules more wisely. If I ever get time I will post them and we can discuss more.

alexlrobertson commented 9 years ago

I've been using ES6 with Angular for about three months and its been a mixed bag. The overall gains from switching to ES6 were great.

However, I ran into some issues with using native classes as controllers and found the duplication of module behavior frustrating. I've written off ES6 until Angular 2, mostly due to how much Angular already provides to solve the problems ES6 tackles.

philmerrell commented 9 years ago

@johnpapa I'd be very interested to see what patterns you come up with using ES6 or TypeScript in addition to your tooling and task automation workflow.

I tend to agree with @alexlrobertson. After some evaluation, I don't think there's a distinct advantage to using ES6 with Angular 1.x. other than it possibly being a step to migrate from 1.x to 2. I'm excited by the new language features ES6 provides (and tooling support via TypeScript), but it seems like there is still some maturing that needs to take place. That being said, I can't wait to use it, and I'm using it in all of my personal projects.

Maybe this is getting off topic, but after the dust settles I'll be interested to see where people land with regard to package management and task automation. I have been really digging JSPM for its one line install jspm install angular and one line require import angular. That is a really nice way to work. It seems like there is a lot of overlap between what jspm can do and what you can do with the combination of npm/bower/grunt or gulp. I'd jump at the opportunity to simplify this process and use one tool instead of many if it can provide the same workflow with regard to package management, transpiling, packaging for production, and running unit tests.

kenkouot commented 9 years ago

Glad to see this discussion happening here as right now, using ES6 and angular.module conventions feel a bit at odds. Let's say I have a module, app.widgets and I want to register all my widget-y directives on this module. Using ES6, you might be inclined to do something like this:

// app.module.js
import angular from 'angular';
import myWidget from './components/myWidget';

export default angular.module('app.widgets', [
  myWidget.name
]);

and:

// myWidget.directive.js
import app from '../app.module';

// app will be undefined, referenced before export
export default app.directive('myWidget', /* ... */);

In the above example, the intention is to use angular.module to group related functionality, but as @philmerrell points out in his first post, there is no great way to do so without chaining .controller, .directive, etc.., declarations onto the original .module declaration and importing files containing only the business logic. I've seen some other clever solutions that involve exporting a function from ES6 modules to defer lookups to angular.module but that also feels pretty dirty.

philmerrell commented 9 years ago

@kenkouot - importing/exporting the module definitions is the direction I first went down when exploring, but as you say, it feels dirty and is a large deviation from the practices established in this style guide. More than anything I felt like simplicity and readability suffered as a result.

An option that would get you out of chaining multiple controllers in the module definition file would be to try option 2 that @johnpapa mentioned above–use the angular module getter syntax in your controllers, directives, services etc...

// angular is defined globally, so no need to import it
angular
  .module('app.cart')
  .controller('CartCtrl', CartCtrl);

class CartCtrl {
  constructor(SomeService) {
    this.SomeService = SomeService;
    this.items = [];
  }

  getItems() {
    return this.SomeService.getItems(results => {
      this.items = results.data;
      return this.items;
    });
  }
}

export default CartCtrl

There are two disadvantages I see to this approach, but neither is a big deal.

First, I really like the idea of Controllers and Services just being plain old javascript with no references to angular. It feels and looks very clean to me. You could also say that this approach makes the controller more reusable and could be used outside of angular, but that's not something I'm considering. The biggest thing probably is that since the angular module system is going away in ng2 it seems more future ready.

Second, if you use the angular module getter syntax in a controller or service (like the above code demonstrates) you have to ensure that your app loads the module definition files first. This is typically handled in a grunt/gulp task that loads scripts starting with *.module.js before other scripts in the index.html file.

When you wire up all of a module's assets in the angular module definition file, the order that the scripts are loaded doesn't matter. Of course it may come at the cost of chaining many controllers, services, etc to one module definition.

skiant commented 9 years ago

After toying a bit with Angular 1.X and ES6, I've come to the conclusion that the Class syntax was too much of a hassle, so I kept the good ol' function.

On the other hand, using import was a really great way to simplify the build process, and added the nice side-effect of forcing myself to break up my app into modules, so i'd avoid writing massive import blocks.

in the end, my most bloated one was looking like this : 

import angular from 'angular';
import directive from './directive.js';
import webcamService from './webcamService.js';
import socketService from './socketService.js';
import templateCompositionService from './templateCompositionService.js';

export default angular.module('booth', [])
.directive('booth', directive)
.service('webcamService', webcamService)
.service('socketService', socketService)
.service('templateCompositionService', templateCompositionService);

Of course arrow functions, let/const, and so on are easy to use and don't have any real drawback that I could see.

mcMickJuice commented 9 years ago

I've been on a client project for about 2 months where we introduced jspm, es6 and a component approach to writing an angular app. I'm also in the middle of rewriting a pet project app using this paradigm. I really dig the es6 syntactic sugar I get with writing classes with getters and setters when writing services as it makes client usage a lot easier. I agree with what some said above about having to map angular elements (controllers, directives, etc.) to a module via a index/module file, but I personally like the fact that I can write plain javascript code for testing purposes and not have to have my unit tests depend on angular or know about angular (unless you are using an angular dep in your code...which can get complicated).

Furthermore, I think that its good to get in this mindset given whats to come in future frameworks. It's early but the approach of importing dependencies and exporting app elements will be a staple in Angular 2 and Aurelia.

Regardless of where you register your angular entities, I think we can all agree though that module loading is the way to go. Whether you use webpack, browserify or are brave enough to tackle JSPM, having your code accessed via a module system is far superior to worrying about handling loading scripts in a certain order or even adding them to your index page via a build task.

Edit: re: to the component approach, here's a helpful starter project if one is looking into organizing an angular app using components - https://github.com/angular-class/NG6-starter

vigie commented 9 years ago

Glad to have found this conversation; I am building a large typescript angular 1.x app with es6 modules and have been thinking about their relationship to angular modules. My thoughts:

The co-existence of two module systems is redundant and confusing, this everyone agrees on. It is not by design and the long term solution is well known - the angular module system will be retired in favor of es6 modules. However, if you are writing angular 1.x with es6 modules today, you are forced to live with both. What's the most sensible strategy?

As much as possible, I want to forget that two module systems live in my app and just work with one. The obvious choice is to pick future proof es6 modules and attempt to drop the angular module system. The only reason I can't drop it entirely is that it holds a couple of essential apis - service() and directive() mainly - for registering angular entities. I also need one module for bootstrapping. Therefore, I must register at least one angular module - my app module. After this, there is no advantage to registering additional angular modules. The rest of my code is modularized and dependencies encoded directly via es6 imports and exports. Creating a parallel track of angular modules introduces no benefit and means I have to maintain a superfluous dependency tree in my code and head. If anyone can think of a reason to use multiple angular modules in an app using es6 modules, please correct me.

Any time I need to register a service or directive, I register it on that sole angular app module. In addition, when I register a service/directive, I do so in the file defining the service or directive. That is to say, I choose option 2 wrt how @johnpapa categorized the options above. Why? Because if you import and register everything in one place, you lose one of the great advantages es6 modules give you - namely lazy loading. You essentially force your entire app to load up front, like the old days. (This applies even if you choose to define multiple angular modules. You force the entire module to load even if all you needed was one directive.)

Now, I understand this point might be moot right now as we are still webpacking and systemjsbuilding or whatever to produce one file. But the other way just feels architecturally wrong to me. It is your service that depends on the service() api in order to become an angular service. From an es6 modules point of view reversing that dependency makes no sense. It only makes sense if you insist on structuring your app using angular modules, but once you decide to leave them behind, the es6 app becomes much cleaner, more flexible and easier to reason about.

The cost is that you get a proliferation of the module api across your service and directive files which is a disadvantage as @philmerrell originally pointed out. But it is a much smaller cost than losing the potential for lazy loading and having to think in terms of angular modules, IMO. The affected files end up with one line at the bottom of them like module.service('myService', MyService) which could be easily removed if the time ever came.

A couple more stray observations:

Disclaimer: the above is a mixture of things I have done and things I intend to do. Would love help in discovering issues with the above pattern.

MikeJoyce19 commented 9 years ago

@vigie interesting approach regarding "bare imports" and having controllers be in charge of what directives they utilize.

seangwright commented 9 years ago

I've been following this discussion and working on converting an Angular 1.x app in es5 to Angular 1.x in es2015 with JSPM/SystemJS.

I followed this and Todd's style guide pretty closely when I began building the app and they have served me very well. But now I'm running into situations and questions that they do not answer.

One specific question I have is how should the angular global object be handled. I want to avoid global objects as much as possible.

With es2015 modules I am declaring an import statement at the top of my module file

/// module.js ///
import angular from 'angular'; // This is defined by jspm map in config.js

import commonServiceModule from 'app:commonService'; // These are defined in jspm config.js maps
import itemServiceModule from 'app:itemService';

import noteManagerService from './noteManager/service'; // These are relative path imports
import noteWizardService from './noteWizard/service';

let moduleName = 's1.note.service';

let noteServiceModule = angular.module(moduleName, [

    commonServiceModule.name, 
    itemServiceModule.name])

    .service(noteManagerService.name, noteManagerService.service)
    .service(noteWizardService.name, noteWizardService.service);

export default noteServiceModule;

And my service class definition looks like this

/// noteManager/service.js ///
import angular from 'angular'; // Should I use this import syntax?

class NoteManagerService {
    /* @ngInject */
    constructor($q, angular, NoteModel, itemManager) { // or should I use this dependency injection?
        this.note1 = new NoteModel("A Note");
        this.note2 = angular.copy(this.note1);
    }
}

export default { service: NoteManagerService, name: 'NoteManagerService' };

But within a controller or service class definition I sometimes use angular.copy or angular.isDefined and was wondering if it would be better to try and inject the global angular object as a parameter of the class constructor or use the import angular from 'angular'; syntax above.

I haven't ever seen anyone inject the angular object into a function via angular's dependency injection and I'm not sure this is even possible.

zachlysobey commented 9 years ago

I'm curious what people's thoughts on the following style I've been using off-and-on recently.

Specifically, I'm putting the controllerAs controller name into a const, so I can reference it using templated strings in my templates, watch expressions, etc...

(function() {
    'use strict';

    angular
        .module('dkProject.projectEditor')
        .directive('myDirective', myDirective);

    const controllerName = 'ctrlName';

    const template = `
        <h2>${controllerName} Template</h2>
        <p>{{${controllerName}.myProperty}}</p>
    `;

    /* @ngInject */
    function myDirective() {
        const directive = {
            bindToController: true,
            controller: MyDirectiveController,
            controllerAs: controllerName,
            scope: {
                myProperty: '='
            },
            template: template
        };
        return directive;
    }

    /* @ngInject */
    function MyDirectiveController($scope) {
        $scope.$watch(`${controllerName}.myProperty`, someFunc);
    }
})();
yadielar commented 9 years ago

What are your thoughts on dependency injection in ES6 class constructors?

Here's what I've seen in most examples around the web:

export class MyController {
  constructor ($log, $timeout, MyService) {
    // Make injected dependencies available to class methods
    // Prefix with underscore to designate them as "private"
    this._$log = $log;
    this._$timeout = $timeout;
    this._MyService = MyService;

    // Scope properties
    this.hasSomething = true;
    this.someString = 'Foo';
  }

  // Scope method
  myMethod() {
    this._MyService.doSomething();
    this._$log.debug('Must use injected deps as pseudo-private methods of the class');
  }
}

MyController.$inject = ['$log', '$timeout', 'MyService'];

I don't know if it's that I'm used to the simple way in which passing parameters to regular constructor functions practically gives you private members to the rest of the class, but it just feels too redundant and yet another place to manually define injected dependencies. Not to mention the fact that they're not really private, though that doesn't bother me too much.

There's another approach involving WeakMaps to solve the privacy problem, but it requires even more boilerplate. But as I already mentioned, I don't mind the pseudo-privacy that much.

I would like to hear your thoughts on this. Am I alone in feeling that it's too much boilerplate, or am I just too enamored with the elegant syntax used in this styleguide for es5?

Of course, if somebody has come up with other approaches please share as well.

zachlysobey commented 9 years ago

For my money, I haven't seen an example yet of DI with the class syntax that I like. Too much boilerplate and public privates seem to be a common theme. I can deal with the latter more than the former, but I still prefer the ES5 syntax put forward in this guide.

seangwright commented 9 years ago

I tried the Class syntax with angular 1.4, but the public-private properties were too much to work with. The function syntax is so much more natural. I'm not against using classes, but until we can get easier global access to DI'd components within a class, without having them accessible via the class's public API, I'm going to avoid it.

That said, the module definition approach I outlined a couple posts above has worked very well for me with the lovely benefit of keeping my Controller/Service/Model classes very clean, no angular boilerplate, no globals, only DI'd values in the function.

TheLarkInn commented 9 years ago

I agree with @mcMickJuice that the component structure for NG6-starter is great, but the only difference I have adopted is that modules are called using the same angular setter/getter variable and it doesn't have to be thrown around all throughout your app. By simply passing it through your exports us can use it to assign directives/controllers/etc.

// index.js (Entry File) 

import angular from 'angular';
import appModules from './modules';
import routerConfiguration from './app.config';

const myApp = angular.module('myApp', ['ui.router']);

// Modules Entrypoint
// Angular Module is passed down through each
// module eliminating need for angular setter/getter
appModules(myApp);

// Router Configuration
routerConfiguration(myApp);
// modules/index.js

import testModule from './test.component';
import headerModule from './header.component'; 

export default myApp => {
  testModule(myApp);
  headerModule(myApp);  
}
// test/test.component.js

import TestDirective from './test.directive.js'
// instead of new angular getter/setter the same app instance is passed down. 
// import angular from 'angular';
// angular.module('myApp').directive()

export default myApp => {
  myApp.directive('directive', TestDirective);
}

This makes your components more modular as there isn't an explicit module being set, instead passing the module you want to 'set' them to.

I have a very rough example of a starter es6+angular+webpack repo that shows this type of module instantiation. (PS. Still a work in progress cleaning out old code from it but its a working loadable repo).

RobbieTheWagner commented 8 years ago

I'm late to this discussion, but I converted the style guide to ES6, specifically with regards to generator-gulp-angular and using ESLint. Please feel free to contribute! https://github.com/rwwagner90/angular-styleguide-es6

zachlysobey commented 8 years ago

@rwwagner90 I took a brief look, and I see quite a few problems and things that would warrant additional discussion. But you don't have issues enabled so I cannot easily comment on them

RobbieTheWagner commented 8 years ago

@zachlysobey sorry about that. I didn't mean to have issues disabled. I guess it's the default if you fork something. I have enabled them. Please feel free to start up some discussions!

ehartford commented 8 years ago

@johnpapa wondering if there is a "right way" now? We are currently grappling with this very question.

RobbieTheWagner commented 8 years ago

@ehartford I would recommend discussing the right way and helping build standards for ES6 over at my fork. https://github.com/rwwagner90/angular-styleguide-es6

fesor commented 8 years ago

@rwwagner90 maybe:

/* recommended */

// app.module.js
import { someService } from './some.service';

angular
    .module('app', ['ngRoute'])
    // .controller... should not be recommended
    .service('someService', someService); // since this is service, not factory

And not to use stand-alone controllers. But this is my five cents...

RobbieTheWagner commented 8 years ago

@fesor I am happy to discuss any suggestions you may have over on the ES6 fork. Please create issues or PRs there.

roelbarreto commented 8 years ago

Is this code fine?

/* @ngInject */
function /* public */ PublicClass($log) {

  class /* private */ _PrivateClass {
    constructor() {
      $log.info('Hello world! Please set a Name!')
      this.name = ''
    }
    setName(name){
      this.name = name
      $log.info('Yo got name! '+name)
    }
  }

  return new _PrivateClass()
}

export default PublicClass

You can access on all dependency injection as normal and you can benefit the es6 class as normal as well, but I'm not sure if it is acceptable and a good practice

peebeebee commented 8 years ago

@ipiz That seems fine:

Maybe an other solution is to use destructuring?

let dep1, dep2;

class Controller {
  constructor($dep1, $dep2) {
    [dep1, dep2] = [$dep1, $dep2];
  }
  someFunction() {
    dep2.doSomething();
  }
}
export default Controller
peebeebee commented 8 years ago

I like @ipiz 's code the best imho.

It could be very concise and clean like

export default [$log, ($log) =>
  new class SomeComponent {
    constructor() {
      $log.info('Hello world! Please set a Name!')
      this.name = ''
    }
    setName(name){
      this.name = name
      $log.info('Yo got name! '+name)
    }
  }
]