This repository has been archived by the owner. It is now read-only.

docs(style-guide): add A2 style-guide - v.1 #1040

Merged
merged 1 commit into from Apr 13, 2016

Conversation

Projects
None yet
7 participants
@wardbell
Contributor

wardbell commented Apr 2, 2016

No description provided.

@googlebot googlebot added the CLA: yes label Apr 2, 2016

@googlebot

This comment has been minimized.

googlebot commented Apr 2, 2016

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

1 similar comment
@googlebot

This comment has been minimized.

googlebot commented Apr 2, 2016

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

@googlebot googlebot added CLA: no and removed CLA: yes labels Apr 2, 2016

@wardbell

This comment has been minimized.

Contributor

wardbell commented Apr 2, 2016

John and I are working on this together. He's ok with this commit.

@wardbell wardbell removed the CLA: no label Apr 2, 2016

@googlebot googlebot added the CLA: no label Apr 2, 2016

@wardbell wardbell added CLA: yes and removed CLA: no labels Apr 2, 2016

@wardbell wardbell changed the title from docs(style-guide): add A2 styleguide - initial commit to [WIP] docs(style-guide): add A2 styleguide - initial commit Apr 2, 2016

@googlebot googlebot added CLA: no and removed CLA: yes labels Apr 8, 2016

**Why?** Unique names help avoid module name collisions. We import and export modules by the file in which they are contained. Thus the name of the files and their folders is important.
**Why?** Names of folders and files should clear convey their intent. For example, `app/speakers/speaker-list.component.ts` may contain a component that manages a list of speakers. The folder and file names clearly convey the meaning, and thus the module import is clear as `import { SpeakerComponent } from './speakers/speaker-list.component'`.

This comment has been minimized.

@IgorMinar

IgorMinar Apr 11, 2016

Member

clear -> clearly

This comment has been minimized.

@IgorMinar

IgorMinar Apr 11, 2016

Member

shouldn't this be updated with the index.ts info which change the import to from './speakers'?

This comment has been minimized.

@johnpapa

johnpapa Apr 11, 2016

Contributor

yes

:marked
## Modules
### Avoid Naming Collisions

This comment has been minimized.

@IgorMinar

IgorMinar Apr 11, 2016

Member

I'm confused about how are you avoiding naming collisions. ES module system is designed to prevent name collisions.

This comment has been minimized.

@johnpapa

johnpapa Apr 11, 2016

Contributor

this is gone

<a id="020"></a>
#### Style 020
- Use unique naming conventions.

This comment has been minimized.

@IgorMinar

IgorMinar Apr 11, 2016

Member

the conventions themselves should be unique? unique for each project or each file? that sounds a bit counter-productive to our goal. :)

This comment has been minimized.

@johnpapa

johnpapa Apr 11, 2016

Contributor

this is removed

<a id="033"></a>
#### Style 033
- Place properties up top, then public functions, then private functions, alphabetized, and not spread through the component code.

This comment has been minimized.

@IgorMinar

IgorMinar Apr 11, 2016

Member

public functions -> public methods
private functions -> private methods

what does "not spread through the component code" mean?

This comment has been minimized.

@IgorMinar

IgorMinar Apr 11, 2016

Member

also keep in mind that ES6 doesn't have property initializers yet. So unless you are using some custom babel plugins, you'll have to initialize the properties in the constructor.

This comment has been minimized.

@johnpapa

johnpapa Apr 11, 2016

Contributor

revised. also fixed the ctor initializer.

for now just using TS

};
private _toastElement: any;
// public properties

This comment has been minimized.

@IgorMinar

IgorMinar Apr 11, 2016

Member

is it intentional that public properties are below private properties? with methods the order is reversed.

This comment has been minimized.

@johnpapa

johnpapa Apr 11, 2016

Contributor

mistake. fixed ordering

- Defer logic in a component by delegating to services.
- Define a component for a view, and try not to reuse the component for other views. Instead, move reusable logic to factories and keep the component simple and focused on its view.

This comment has been minimized.

@IgorMinar

IgorMinar Apr 11, 2016

Member

this sounds weird to me. In my mind there are two kinds of components:

1/ routable components that are usually very application specific and are not meant to be reused even within the application.
2/ reusable components (application agnostic ones as buttons, or the whole material suite, and application-specific ones like profile/product widgets with pop-overs, etc).

the purpose of the service is to facilitate data delivary to these components

- Define a component for a view, and try not to reuse the component for other views. Instead, move reusable logic to factories and keep the component simple and focused on its view.
**Why?** Logic may be reused by multiple components when placed within a service and exposed via a function.

This comment has been minimized.

@IgorMinar

IgorMinar Apr 11, 2016

Member

functions -> methods?

This comment has been minimized.

@johnpapa

johnpapa Apr 11, 2016

Contributor

revised

<a id="040"></a>
#### Style 040
- Services are singletons and should be used for sharing data and functionality.

This comment has been minimized.

@IgorMinar

IgorMinar Apr 11, 2016

Member

well, not quite. services are singletons within the same injector - you can think about each element in the component composition tree as having it's own injector and each element inherits injector services from the parent.

but if a child element defines a provider for a service that was already defined an instantiated in the parent, the DI will create a new instance of this service visible only to this child component and its children. This is what allows you to have multiple instance of certain services (e.g. ElementRef is different for each component) and we don't need the concept of "locals" from Angular 1.

This comment has been minimized.

@johnpapa

johnpapa Apr 11, 2016

Contributor

yep. just hadnt figured out how to articulate this, and what is applicable to the guide, if at all. because that depth belongs in the docs, so some of this may just go

<a id="041"></a>
#### Style 041
- Services should be provided to the Angular 2 injector at the top-most component where they will be shared.

This comment has been minimized.

@IgorMinar

IgorMinar Apr 11, 2016

Member

Maybe we should even before this section introduce the concept of component hierarchies as a separate recommendation. Everything in an Angular app is based on this single concept (the component composition, routing, di, directory structure, lazy-loading, compilation units....).

The general guidance for creating reusable components or defining providers should be that they should be created at the level of deepest common parent. This sets the visibility of that component or service instance within the application tree.

This comment has been minimized.

@johnpapa

johnpapa Apr 11, 2016

Contributor

agreed

**Why?** This makes it easier to test (mock or real) the data calls when testing a component that uses a data service.
**Why?** Data service implementation may have very specific code to handle the data repository. This may include headers, how to talk to the data, or other services such as `$http`. Separating the logic into a data service encapsulates this logic in a single place hiding the implementation from the outside consumers (perhaps a component), also making it easier to change the implementation.

This comment has been minimized.

@IgorMinar

IgorMinar Apr 11, 2016

Member

$http is no more :)

This comment has been minimized.

@johnpapa

johnpapa Apr 11, 2016

Contributor

good catch :)

import { Component } from 'angular2/core';
import { RouteConfig, ROUTER_DIRECTIVES, ROUTER_PROVIDERS } from 'angular2/router';
import { SpeakersComponent, SpeakerService } from './^speakers';

This comment has been minimized.

@IgorMinar

This comment has been minimized.

@johnpapa

johnpapa Apr 11, 2016

Contributor

revised

import { RouteConfig, ROUTER_DIRECTIVES, ROUTER_PROVIDERS } from 'angular2/router';
import { SpeakersComponent, SpeakerService } from './^speakers';
import { DashboardComponent } from './^dashboard';

This comment has been minimized.

@IgorMinar

This comment has been minimized.

@johnpapa

johnpapa Apr 11, 2016

Contributor

revised

1. `L`ocating our code is easy
2. `I`dentify code at a glance
3. `F`lat structure as long as we can
4. `T`ry to stay DRY (Don’t Repeat Yourself) or T-DRY

This comment has been minimized.

@IgorMinar

IgorMinar Apr 11, 2016

Member

why are you repeating yourself? this is the same as line 548. no?

This comment has been minimized.

@johnpapa

johnpapa Apr 11, 2016

Contributor

gone

**Why?** One component per file avoids hidden bugs that often arise when combining components in a file where they may share variables, create unwanted closures, or unwanted coupling with dependencies.
The following example defines the AppComponent, handles the bootstrapping, and shared functions all in the same file.

This comment has been minimized.

@mgechev

mgechev Apr 12, 2016

Member

"AppComponent" could be formatted as a keyword AppComponent.

This comment has been minimized.

@johnpapa

johnpapa Apr 12, 2016

Contributor

done

export class ToastComponent implements OnInit {
// public properties
title: string;
message: string;

This comment has been minimized.

@mgechev

mgechev Apr 12, 2016

Member

title should be after message (title > message in lexicographical order, not alphabetized).

This comment has been minimized.

@johnpapa

johnpapa Apr 12, 2016

Contributor

done

]
})
@RouteConfig([
{ path: '/dashboard', name: 'Dashboard', component: DashboardComponent, useAsDefault: true },

This comment has been minimized.

@mgechev

mgechev Apr 12, 2016

Member

I think it'll be good to define naming conventions for the routes. I like the current one - the component name without the Component suffix.

This comment has been minimized.

@johnpapa

johnpapa Apr 12, 2016

Contributor

yes. we need a style for it

This comment has been minimized.

@johnpapa

johnpapa Apr 12, 2016

Contributor

but i heard a rumor that route names may be going away. perhaps we avoid any route styles for now?

@wardbell @IgorMinar ????

/widgets
/content
index.html
.package.json

This comment has been minimized.

@mgechev

mgechev Apr 12, 2016

Member

There's extra "." before package.json.

styleUrls: ['app/app.component.css'],
directives: [ROUTER_DIRECTIVES, NavComponent],
providers: [
ROUTER_PROVIDERS,

This comment has been minimized.

@zoechi

zoechi Apr 12, 2016

Adding ROUTER_PROVIDERS on a component instead of to boostrap(AppComponent, [ROUTER_PROVIDERS]) seems to cause issues with routing. I suggested to move them to bootstrap() repeatedly on StackOverflow and most of the time this was what fixed the problem. Should this work the same on the root component?

This comment has been minimized.

@johnpapa

johnpapa Apr 12, 2016

Contributor

tldr; we try not to provide anything at boostrap.

this makes it more difficult to test and often the root component is what really needs the providers, as it is the root of the component tree. this also makes the bootstrap super simple and focuses just on the platform.

i know @wardbell had some chats with Misko on this and he can chime in too.

This comment has been minimized.

@zoechi

zoechi Apr 12, 2016

I don't have a strong opinion in one direction or the other, but I saw this caused lots of confusion. The hint in the docs to use provide: [] on the root component seemed to be interpreted a lot to add it on every component which breaks many things.
I just think the favored approach should be stated very clearly.

@zoechi

This comment has been minimized.

zoechi commented Apr 12, 2016

What about some words about whether to prefer decorators or component/directive parameters (inputs: [], outputs: [], host: {})?

@mgechev

This comment has been minimized.

Member

mgechev commented Apr 12, 2016

@zoechi this will follow-up once we agree on the core principles (directory structure and naming).

@johnpapa

This comment has been minimized.

Contributor

johnpapa commented Apr 12, 2016

@zoechi I agree with @mgechev ... we need to get the core guidelines written first. Then we'll keep updating it.

@wardbell wardbell changed the title from [WIP] docs(style-guide): add A2 styleguide - initial commit to docs(style-guide): add A2 styleguide - v.1 Apr 13, 2016

@wardbell wardbell changed the title from docs(style-guide): add A2 styleguide - v.1 to docs(style-guide): add A2 style-guide - v.1 Apr 13, 2016

@wardbell

This comment has been minimized.

Contributor

wardbell commented Apr 13, 2016

Merging ... THIS BRANCH IS NOW CLOSED

@wardbell wardbell merged commit bd07936 into angular:master Apr 13, 2016

1 check failed

cla/google CLAs are signed, but unable to verify author consent

@wardbell wardbell deleted the IdeaBlade:docs-style-guide branch Apr 13, 2016

export class AppComponent implements OnInit{
heroes: Hero[] = [];
constructor(private _heroService: HeroService) {}

This comment has been minimized.

@choeller

choeller Apr 18, 2016

Just out of curiosity: Why do you still recommend to prefix private fields with _. I always saw that as an ugly hack because of the lack of the private modifier, so since using TypeScript I totally dropped that.

This comment has been minimized.

@mgechev

mgechev Apr 18, 2016

Member

Mostly for debugging, especially when you don't have sourcemaps.

Also, you can access all these properties inside of the templates. Having instance members prefixed with _ gives better semantics.

This comment has been minimized.

@choeller

choeller Apr 18, 2016

@mgechev Thanks for the quick reply! I just added a discussion-issue about this here: #1108. As this convention goes against Microsofts recommendations I think that it is at least worth to think about dropping the prefix ;)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.