Code style guide for components authoring

[Blackbaud-SteveBrush]

We need to provide some documentation around code style so that we have a constitution (of sorts) we can point to during development.

This document should include:

• How HTML templates should be formatted
• How Input and Output properties should be named, handled
• How import statements are written

[Blackbaud-SteveBrush]

HTML component tags should be displayed as:

<component-name *structuralDirective="can only be one per tag"
  nativeattributes=""
  unboundProperty=""
  [inputs]=""
  [(twoWay)]=""
  (outputs)=""
  #templateReferenceVariables
>
  Some content here.
</component-name>

Note: Since an element can only have one structural directive, we place those on the same line as the tag name.

Note: Thus rule should be followed even if there is only one argument unless that one argument is a structural directive

[Blackbaud-SteveBrush]

Imports

• All imports should be listed on a new line.
• Wrap your imports in "region" folding markers.

// #region imports
import {
  Component,
  Input
} from '@angular/core';

import {
  SkyAppConfig
} from '@blackbaud/skyux-builder/runtime';

import {
  Observable
} from 'rxjs/Observable';

import 'rxjs/add/observable/of';

import {
  Subject
} from 'rxjs/Subject';

import {
  FooComponent
} from './foo.component';

const foo = require('bar');
// #endregion

Order of imports
Imports should be listed first by "depth" and second, alphabetically. The top-most imports should be third-party, followed by imports that are more "local" to the current file.

[Blackbaud-SteveBrush]

Class properties

@Input()
public someInput: string;

@Input()
public set baz(value: string) {
  this._baz = value;
}

public get baz(): string {
  return this._baz;
}

@Output()
public someOutput: EventEmitter<void>;

public foo = false;

@ViewChild('someElement')
private someElement: ElementRef;

private bar = 'string';

// List private fields (that are accessed via getters/setters) under all other private properties.
private _baz: string;

[Blackbaud-SteveBrush]

Constructors:

• Constructors should list their arguments each on a new line.
• Constructors do not need an access modifier.
• Arguments should be listed in order of public, private, optional (decorators on the same line).

constructor(
  public element: ElementRef,
  private renderer: Renderer2,
  @Optional() private foo?: Foo
) { }

Note: If a new parameter is added to an existing constructor (or public method), it must be added as @Optional() foo?(or, foo?) and at the end.

Note: Thus rule should be followed even if there is only one argument

[Blackbaud-SteveBrush]

Observables:

Class properties that represent Observable values should be suffixed with Stream:

public get messageStream(): Observable<string> {
  return this._messageStream;
};

private _messageStream = new BehaviorSubject<string>('');

public someMethod() {
  this._messageStream.next('');
}

[Blackbaud-SteveBrush]

Events/Outputs:

Events that have happened (and will never happen again) are named in the past tense, and are completed immediately after firing:

this.closed.emit();
this.closed.complete();
// ...
someComponent.closed.subscribe(() => {
  // No need to unsubscribe from a past-tense observable :)
});

If the event is intended to be fired multiple times, the property name should be in present tense:

this.save.emit({});
// ...
someComponent.save
  .subscribe(() => {
    // This subscription will fire forever...
  });

If the Observable property is a value (and not an event), the property name should be suffixed with Stream (see previous comment above).
NOTE: Never make Inputs observables!

public someValueStream = new Subject<string>();
// ...
someValueStream.next('hello!');

[Blackbaud-BobbyEarl]

Hey @Blackbaud-SteveBrush I really appreciate you taking the ball to get some of these documented. Did you have any thoughts about the best way to provide feedback?

I'm happy to leave in the form of comments to this issue, just wanted to sync up before doing so.

[Blackbaud-SteveBrush]

Methods

• Always declare return type (never any).
• If the method has two or more arguments, place each one on a new line.
• JSDoc for public methods.

/**
 * This method does something
 * @param foo - Some description here
 * @param bar - Some description here
 */
public someMethod(
  foo: string,
  bar: boolean
): void { }

[Blackbaud-SteveBrush]

Strings

If the string is longer than the max line length:

const foo = 'foo' +
  'bar' +
  'baz';

Multi-line:

  const foo = 
`Foo bar ${baz}
Some more stuff.`;

[Blackbaud-SteveBrush]

Code blocks:
BAD:

if (true) return;

GOOD:

if (true) {
  return;
}

[Blackbaud-SteveBrush]

Unsubscribing from observables

See: https://stackoverflow.com/questions/38008334/angular-rxjs-when-should-i-unsubscribe-from-subscription#answer-41177163

private ngUnsubscribe = new Subject();

public ngOnInit(): void {
  Observable.fromEvent(...)
    .takeUntil(this.ngUnsubscribe)
    .subscribe(...);
}

public ngOnDestroy(): void {
  this.ngUnsubscribe.next();
  this.ngUnsubscribe.complete();
}

[blackbaud-johnly]

Notes from discussion with @Blackbaud-SteveBrush and @blackbaud-conorwright based on Conor's notes from his initial SKY UX contributions:

Conor's initial feedback: All components are supposed to be using changeDetection: ChangeDetectionStrategy.OnPush that should be documented somewhere for contributors.

Notes from follow-up discussion: This bleeds into style guide. It’s more implementation than style, but Steve wants to finalize the style guide and have a section on implementation details as well, and then link to that from the contribution guidelines. For normal contributions, we don’t really look at the style for acceptance criteria. We just check the public API and handle style after the fact.

[Blackbaud-AlexKingman]

Closing - moved to internal SKY team docs site.

[blackbaud-conorwright]

Alex moved it to: https://docs.blackbaud.com/skyux-team-docs/style