toddmotto / angularjs-styleguide

AngularJS styleguide for teams
https://ultimateangular.com
5.97k stars 702 forks source link

First thoughts #53

Closed bcarroll22 closed 8 years ago

bcarroll22 commented 8 years ago

Todd, this is awesome. I love to see these types of things being written in the Angular community. It's challenging, it's much needed, and it's refreshing. I have a few thoughts after reading through it and thinking about some of the things my team went through when building an ng 1.5 app using these principles and similar architecture.

Module exports

First, is there a reason not to just export the name from the module? It looks cleaner in the module setter, and has the same functionality. Instead of having to do:

import {Module} from './module';

angular.module([ Module.name ]) 

you could just do:

import { Module } from './module';

angular.module([Module])

My team think the latter snippet looks cleaner (for a couple reasons). There's not much benefit to doing it either way though, I suppose.

Naming convention

We mostly follow the naming pattern suggested here, but lately I've been wondering if it is necessary to name things like calendar.controller.js when Cmd+P in Atom, Sublime, etc. can find the controller in the calendar folder for you. Just a thought, but it would really cut down on the amount of typing you do for file names over time.

Binding readability

We found that having a utility file that exports a const for each of the binding types makes it easier to see what types of bindings are being used, and it's a little less "cypherish". Really helps with file readability. It's basically:

export const CALLBACK = '&';
export const ONE_WAY = '<';
export const STRING = '@';

When writing apps with ES2015+, it's inevitable to need a build system. With that in mind, is it smarter to encourage the use of $inject or to strongly encourage ngAnnotate? Much less manual work, and much less error-prone with strict-di. I think it's better to let ngAnnotate do the heavy lifting, and it works with all the major build systems.

Class methods or pure functions?

From our experience, any pure functions that are not bound directly to our classes are easier to test in isolation. It's annoying to have to mock the controller and use ng-mocks to mock dependencies to test simple actions when you don't have to. Granted, we've tried to keep most of our functions are pure, and we've managed to rely very little on Angular services and dependency injection. It's been really nice and it's helped us a lot with writing tests and reusing code. Plus, mocha can run 600+ tests as quickly as you save a file. Karma can run 600+ tests about as fast as you can download angular 2 on a bad internet connection.

Lifecycle order

Another thing; one gotcha that got us over and over was that, somewhat inexplicably, $onChanges gets called before $onInit. I don't know where it's relevant to point that out, but it should probably be pointed out nonetheless.

Single-file components?

What's the benefit of defining the template inline, but not the controller? why not do:

const TodoComponent = {
  bindings: {
    whatever: '<',
  },
  controller: class TodoController {
    constructor(TodoService) {
      'ngInject';
      this.TodoService = TodoService;
    }
    $onInit() {
      this.newTodo = {
        title: '',
        selected: false
      };
      this.todos = [];

    this.TodoService.getTodos
      .then(response => this.todos = response);
    }
    addTodo(event.todo) {
      if (!event.todo) return;
      this.todos.unshift(event.todo);
      this.newTodo = {
        title: '',
        selected: false
      };
    }
  },
  template: `
    <div class="todo">
      <todo-form 
        todo="$ctrl.newTodo"
        on-add-todo="$ctrl.addTodo($event);">
      <todo-list 
        todos="$ctrl.todos"></todo-list>
    </div>
  `
};

We found that cutting file count whenever possible tends to help DX on a day-to-day basis. We've even tinkered with putting everything, even module definition, in one file. It tends to be kind of nice if you're keeping your components small.

Classes or arrow functions

That non-class directive pattern really seems a lot more natural to me. I know, I know, ng2. But still. Look how nice that looks, right? So clean, so little instantiating a class just to get the same functionality ¯\_(ツ)_/¯

File structure

Here's how we organize files. Not really suggesting this be the structure, just an another look at organization, may be helpful to someone:

├── client/
|  ├── app/
|  |  ├── redux/
|  |  |  └── modules/
|  |  |    └── events-signup/
|  |  |      ├── index.js
|  |  |      ├── selectors.js
|  |  |      └── state.js
|  |  └── scenes/
|  |  |  └── events-signup/
|  |  |    ├── index.js
|  |  |    ├── events-signup.controller.js
|  |  |    ├── events-signup.route.js
|  |  |    ├── events-signup.html
|  |  |    └── events-signup.scss
|  |  ├── index.js
|  |  └── reducers.js
|  └──global/
|     ├── core/
|     |  ├── core.config.js
|     |  ├── core.run.js
|     |  └── index.js
|     ├── components/
|     |  ├── nav/
|     |  |  ├── nav.js
|     |  |  ├── nav.scss
|     |  └── index.js
|     ├── scenes/
|     |  └── layout/
|     |     ├── index.js
|     |     ├── layout.route.js
|     |     ├── layout.scss
|     |     └── layout.controller.js
|     └── utils/
|        ├── bindings.js
|        ├── env.js
|        └── ng-utils.js
└── index.html

The reason global lives outside of app is the it contains a lot of utilities and boilerplate that isn't "app" specific, and could very easily be used by another application we want to quickly start building.

Also, we keep all state management (in our case, Redux) code outside of the components. If state is normalized correctly, it should be a 1:1 mapping to components, it should behave more like a database. We've just found this structure to help us think of our data independently of our components. We called all of our routes scenes I think just because of some React/Redux article I read somewhere or something, no benefit to scenes over routes

Require vs. bindings

Could we better explain the benefit of passing a binding instead of just requiring the parent? Devil's advocate: Why would i go through the long process of setting up all those bindings for one-way communication when I could just say require: '^^parentComponent' and accomplish the same thing, and cover all my bases in one line of code instead of 10+?

Using $event as the payload of & bindings

Using $event === smart. I love it, and wish I would've been doing that all along. It's much better to have that consistent API for outputs.

Use resolves to make smart components

Shoutout to Chris Thielen for this idea, but in a lot of cases, when you're routing straight to components, there's a good chance that most of the bindings you'd do in smart components can probably be done in resolves on a ui-router state. Just one more step that can be cut out.

NG2.0 vs. the world

Finally, I know there's been a lot of thought put into this style guide to help with transitioning to Angular 2. One of the thoughts that came to my head as I read this is: Do we, while catalyzing modern best practices in the community, want to encourage good Angular 2 practices, or good JavaScript practices. One example: there is HUGE benefit to factory functions. They're easy. They're perfect for the types of things we want a service to do. They're very useful. They're gone in ng2. RIP .factory().

I think one of the great things about the React community is that they truly strive to make people better JavaScript devs. To me, that's a useful thing. That transcends framework boundaries, and makes a lasting impact on all of us as developers. I really believe this guide, along with a lot of the things you write, are going to help foster more of that in the ng 1.x community. Thanks for doing what you do, and I'm looking forward to future discussions!

toddmotto commented 8 years ago

Made a few updates based on this, exported the .name property inside the actually component module exports makes more sense IMO too. Also added EventEmitter value wrapper.

Hypercubed commented 8 years ago

Are you still missing a couple of .names, for example in the Component and Common modules?

d3lm commented 8 years ago

@bcarroll22 Regarding your point Class methods or pure functions?, how do share common functionality if you barely use services? In my opinion you should try to keep your component controller's as small as possible and outsource business logic to services. Yes, you have to use ng-mock or something else to mock your dependencies but almost abandon services doesn't sound right to me. Having pure functions is one thing, and I absolutely agree that pure functions are great since they are easy to test and what not, but I would try to relocate most of the logic to services, which use pure functions as well. Maybe I didn't quite understand what you meant, can you give an example? I am curious what your component looks like. What kind of logic do you have within your controllers? What do you use services for?

bcarroll22 commented 8 years ago

We use Redux.

When using Redux, all of our actions are completely pure and the only place we ever have any sort of Angular dependency to inject is in a middleware. I plan on writing more about using Redux with Angular 1.5 components soon, I just haven't had the time to share the successes we've had with it. Our controllers manage to stay mostly non-existent through using Redux.

State management has been incredibly easy with Redux, and the devtools are second to none in my opinion.

bcarroll22 commented 8 years ago

Also whenever it's possible, we try to extract methods from classes so that we can export and test them individually. So for example, instead of doing:

class ExampleController {
  someMethod (param) {
    return param + 1
  }
}

we tend to prefer:

export default const someFunction = param => param + 1

class ExampleController {
  constructor () {
    this.someMethod = someFunction
  }
}

or, if we define the controller in the component definition, we may even do:

export default const someFunction = param => param + 1

const exampleComponent = {
  controller () {
    this.someMethod = someFunction
  }
}
bcarroll22 commented 8 years ago

Think about the benefits of that approach for testing as the controller scales and gets some Angular dependencies. Now instead of having to use ng mocks and stubbing a controller, I can still test (at least some) methods individually.

Obviously, this doesn't always work and there are times when you need to do something like access this, and that's okay, we've just found that it's helpful to use this method whenever we can for testing.

d3lm commented 8 years ago

Ah, didn't know you were using Redux. I could have seen it in your folder structure though. But still you need like action creators to do your API calls. And in Angular 2 you would simply create a service to fetch data from your backend. Would love to see an article on how you did your app design using ngrx in combination with the new component method. I think Angular 1.x has become really nice with all the cool changes such as life cycle hooks, .component, one-way data flow etc. You can design highly scalable and reusable ui chunks. I am also really into using a store and something like ngrx to manage your state. But still, thats just for managing your state. You would need services for instance for business logic such as authentication, logging etc. no? Maybe it's completely different when using a store, and everything can be handled in reducers / action creators (for side effects) and controllers.

bcarroll22 commented 8 years ago

Yeah believe it or not, we've got a large-scale, production application (which does include authentication) and no providers, services, or factories. Absolutely everything is in .component() or redux (with the exception of defining router states in .config()).

We also don't use $http. All of our server calls are done using isomorphic-fetch with redux-effects-fetch. We have special helpers which were often easier than using $http, and now none of our business logic is coupled to an Angular 1.x implementation.

d3lm commented 8 years ago

That sounds really intersting. Unfortunately there won't be a repo that people can look at right? Would love to see examples of your architecture. I could also contribute.

What do you think about creating a starter with the best practices? CC @toddmotto

Hypercubed commented 8 years ago

@bcarroll22, that's something I've been thinking about for a while. When using ES2015 should we skip $http altogether? Is that a best practice now?

bcarroll22 commented 8 years ago

@d3lm I've got some things coming down the pipeline to show some redux usage.

@Hypercubed I can't say for sure what would work best for your team, but I can say it's worked out great for us. Our general mindset was pretty much the following:

  1. React has been doing component-based architecture for a long time now, and there's a lot of content out there about "best practices", so what can we learn from them?
  2. ES modules are an easy to use feature that makes code usage in other files simple, so why not use that whenever possible?
  3. Angular 1.x is sunsetting, and we're not sold on Angular 2, so how can we reduce coupling as much as possible so that we can easily migrate to whatever we deem is the best choice for us in the future?

Using those, and some other smaller things, as guiding principles has helped us immensely as a team.

bcarroll22 commented 8 years ago

Check out this package, I just released it last night. It's in beta, but it's Redux for Angular that leverages component architecture, and is really consistent with how react-redux works. There's an example in the documentation, and I do expect to put more examples and docs up soon, but it should hopefully get you started!

d3lm commented 8 years ago

Why not leverage ngrx/store?

bcarroll22 commented 8 years ago

tl;dr version: We like Redux, we don't want to be tethered to ng1.x, and probably aren't leaning toward a transition to ng2, so bindings like this make the most sense.

There's also a plethora of resources for Redux, as well as a ton of extensions available, and this package will allow you to use not only all of those extensions, but in most case any example code you find as well. Basically the only difference in the todo example in my repo and the react example is the Angular-specific code, which isn't much. Other than that, they're almost entirely the same. That was a benefit we didn't have when starting out, and there were a lot of things we had to map over to the corresponding place.