Angular 2アンチパターン集

gistfile1.txt

2016/12/03 にQiitaに投稿した記事のアーカイブです
  
---

@armorik83です。[Angularアドベントカレンダー](http://qiita.com/advent-calendar/2016/angular)の4日目となる本日は、かつて好評だった拙記事"[AngularJSアンチパターン集](http://qiita.com/armorik83/items/b00818ecaf2e93734b36)"にあやかって、Angular 2（以下、単にAngularと表記したときは2.0以上4未満を指す）について気をつけた方がよい点――アンチパターンをまとめたいと思います。

# Angular 2.xでアンチパターンは起こりうるか
Angular 1系とは大きくAPIが変わったAngular 2系。APIが大きく変わった理由として、Web標準により近い構成を取れるようにする目的がありました。例えば、`angular.module()`ではなくTypeScriptの`import from`をベースとするソース分割の仕組みであったり、ES2015 `class`を標準としたComponentやServiceの定義であったり、`$q`ではなく標準の`Promise`を使ったりなどです。

このようにAPIがよりWeb標準に近づくことによって、それぞれの開発者がバラバラなスタイルで書いてしまう可能性が下がりました。公式のチュートリアルを確認し、[`angular-cli`](https://github.com/angular/angular-cli)が生成するファイルの作法に則っておけば、まず間違いない構成で開発を進めることができます。

ではこれで、Angularを用いて開発する上で何も支障なく進められるかというと、そうでもありません。Angular 1に比べてWeb標準という前提が整いAPIの数も整理されたとはいえ、そのAPIの使い方次第では、あまり望ましくない結果に辿り着いてしまうことがあります。

望ましくない結果は、主にチームで開発する上でのAPI習熟度の差によって起こりやすい、言い換えると見よう見まねで解釈したときに起こりやすい、というのが筆者の経験則です。本稿では筆者の業務経験（Angular 2歴1年）に基づいたアンチパターンを掲載します。

# アンチパターン集
## Componentの初期化処理をconstructorに書く
### 問題点
Componentの初期化処理を`constructor`に書くべきではありません。

```ts
@Component({
  selector: 'my-foo',
  template: `
    {{value}}
  `,
})
export class Foo {
  @Input() value: number
  constructor() {
    console.log('constructor', this.value) // undefined
  }
  
  ngOnInit() {
    console.log('ngOnInit', this.value) // 期待される値
  }
}
```

`@Input()`評価のタイミングは`constructor`より後のため、`@Input()`で受ける値を`constructor`で期待すると`undefined`となります。

https://plnkr.co/edit/X1rrqoMLy54z2jn4lslw

###解決策
Angularでは`ngOnInit()`や`ngOnDestroy()`などといった[Lifecycle Hooks](https://angular.io/docs/ts/latest/guide/lifecycle-hooks.html)と呼ばれるAPIがありますが、このうちの`ngOnInit()`を初期化処理の記述に用いましょう。

`@Input()`が絡まない値は`constructor`内でも記述できますが、「この内容は`constructor`、この内容は`ngOnInit()`」という判断を毎回迫られることになり、特にチームで開発する場合にレビュー基準がブレやすいことから、一律で`ngOnInit()`に書くと決めておいたほうがよいです。

## ComponentのスーパークラスにngOnInitの共通処理を書く
### 問題点
Angular 2からはComponentやServiceを`class`構文で記述できるようになりました。そのため、多くのComponentの共通処理を`extends AbstractComponent`などとして別クラスにまとめたい状況があると思います。

```ts
class AbstractComponent {
  // ...
}

@Component({
  selector: 'my-foo'
})
export class FooComponent extends AbstractComponent {
  constructor() {
    super()
  }
}
```

この場合、注意が必要です。Lifecycle Hooksにある`ng`で始まる名前のメソッドを継承した場合、予期せぬ動きをする場合があります。

```ts
class AbstractComponent {
  ngOnInit() {
    // なにか共通処理
  }
}

@Component({
  selector: 'my-foo'
})
export class FooComponent extends AbstractComponent {
  constructor() {
    super()
  }

  ngOnInit() {
    super.ngOnInit()
  }
}
```

筆者が頻繁にハマったのはRouterが絡んだケースで、大きなアプリケーションになるほど、この継承による思わぬ挙動が発見しづらいバグとして潜みます。

### 解決策
解決策としてはそもそも継承せずに、例えば共通処理を関数として切り出して全てのComponentがその処理を呼ぶ、というのが一番安全です。もし継承したい場合は、`constructor`内やLifecycle Hooksと被らない名前のメソッドだと経験的には問題が起こっていません（保証はできません）。

もしLifecycle Hooks周りの挙動で不思議な動きをした場合、その周辺に`extends`が絡んでいないかを確認するとよさそうです。

## Inputにオブジェクトを渡す
### 問題点
この状況は比較的軽い問題なので行うことも多々あると思いますが、注意が必要です。

```ts
@Component({
  selector: 'my-foo',
  template: ``,
})
export class Foo {
  @Input() primitive: number
  
  ngOnChanges() {
    console.log('this.primitive', this.primitive)
  }
}

@Component({
  selector: 'my-bar',
  template: ``,
})
export class Bar {
  @Input() obj: {value: number}
  
  ngOnChanges() {
    console.log('this.obj.value', this.obj.value)
  }
}

@Component({
  selector: 'my-app',
  template: `
    <my-foo
      [primitive]="primitive"
    ></my-foo>
    <my-bar
      [obj]="obj"
    ></my-bar>
  `,
})
export class App { 
  primitive: number
  obj: {value: number}

  ngOnInit() {
    this.obj = {}
    let i = 0
    let j = 0

    setInterval(() => {
      i += 1
      this.primitive = i
    }, 1000)
    
    setInterval(() => {
      j += 1
      this.obj.value = j
    }, 1000)
  }
}
```

たとえば上のようなケースです。ここでは毎秒値を変えながら`Foo`と`Bar`に渡していますが、毎秒`ngOnChanges()`が発火する`Foo`に対して、オブジェクトを受け取る`Bar`は最初の一度しか`ngOnChanges()`が発火しません。

https://plnkr.co/edit/Kz30iJsKpYSk70yAVR1c

### 解決策
オブジェクトや配列ではなく、プリミティブな値を渡すように修正します。

`ngOnChanges`は参照が変化したときに発火します。すなわち、プリミティブな値（`number`, `string`, `boolean`）が変更されたとき、またはオブジェクト、配列の参照が変わった時に限ります。オブジェクトや配列の中身が変わっただけでは発火しないのです。このため、`ngOnChanges()`が絡む時にはバグの温床となりやすいため、オブジェクト、配列は直接渡さないことを推奨します。

または、次のように書くこともできます。

```ts
@Component({
  selector: 'my-foo',
  template: ``,
})
export class Foo {
  @Input() primitive: number // こうではなく

  ngOnChanges() {
    console.log('this.primitive', this.primitive)
  }
}
```

```ts
@Component({
  selector: 'my-foo',
  template: ``,
})
export class Foo {
  private _primitive: number

  // setterに直接@Input()を付与
  @Input() set primitive(val: number) {
    this._primitive = val
    console.log('this._primitive', this._primitive)
  }
}
```

`@Input()`をプロパティではなく、setterに直接付与することで、このプロパティが変更されたときに限定的に処理を実行することもできます。setter方式の場合は、値を保持したい場合別名のプロパティに再格納する必要があるので、`ngOnChanges()`の引数`SimpleChanges`を使うかsetter方式を使うかは、状況や複雑さに応じて判断してください。

一方で、`ngDoCheck()`というLifecycle Hooksと`KeyValueDiffers`または`IterableDiffers`を組み合わせることでもこの問題を解決できます。ただし、`ngDoCheck()`は非常に高負荷になりやすいAPIであり各Differsの初期化も煩雑であることから、チーム開発時に高い習熟を求められ、経験上あまりお勧めはしません。

## テンプレート内にDIしたServiceをそのまま書く
### 問題点
Angularのテンプレート構文の解釈はよくできており、JavaScriptと同様の動きをしているように見えます。

```ts
@Injectable()
export class Service {
  getValue(): string {
    return 'value'
  }
}

@Component({
  selector: 'my-app',
  template: `
    <p>{{service.getValue()}}</p>
  `,
})
export class App {
  constructor(public service: Service) {}
}
```

たとえばこの`<p>{{service.getValue()}}</p>`は、正しく`Service#getValue()`メソッドが呼ばれ、ブラウザ上には"value"と表示されます。

これは動作するため一見問題ないように見えますが、パフォーマンス上で問題となってきます。テンプレート中に関数呼び出しが書かれると、Change Detection（変更検知）のたびに一度呼び出されてその結果を比較して変更を認識するので、パフォーマンスが劣化するのです。

https://plnkr.co/edit/7xqdry9B1fqZy4Zxx7et

### 解決策

多少面倒でも、一旦プロパティに受けるのが望ましいです。

```ts
@Component({
  selector: 'my-app',
  template: `
    <p>{{value}}</p> <!--DIしたServiceを使わない-->
  `,
})
export class App {
  value: string
  constructor(public service: Service) {}

  ngOnInit() {
    this.value = this.service.getValue() // 一旦受ける
  }
}
```

テンプレート内は文字列なのでTypeScriptのコンパイルで検知できないというデメリットも存在します。ただしこれは、[ngc](https://github.com/angular/angular/tree/master/modules/@angular/compiler-cli) (Angular compiler-cli)を使っていればTypeScriptのコンパイルエラーとして検知できるようになります。

そのため、なにより「[AoT](https://angular.io/docs/ts/latest/cookbook/aot-compiler.html) (Ahead-of-time compilation)を意識せずにAngularの開発を続ける」ことこそがアンチパターンともいえます。

## ComponentのInputに関数を渡す
### 問題点
Componentの`@Input()`に関数を渡すのは避けましょう。`@Input()`には値を与えるようにすべきです。「今回は特例として」などと言いながら書いてしまっても、数ヶ月もすれば何故そう書いたのか分からなくなるものです。

```ts
@Component({
  selector: 'my-foo',
  template: ``,
})
export class Foo {
  @Input() callback: Function
  ngOnInit() {
    this.callback('hello')
  }
}

@Component({
  selector: 'my-app',
  template: `
    <my-foo [callback]="callback"></my-foo>
  `,
})
export class App {
  callback: Function

  ngOnInit() {
    this.callback = (str: string) => {
      console.log('this.callback', str) // hello
    }
  }
}
```

https://plnkr.co/edit/IgcglJ9E0KLUEZlksgEp

### 解決策
素直に`@Output()`で実装できないか検討しなおすのがよいでしょう。どうしても`@Output()`では実現できない場合、それはAngularとして複雑なことをやろうとしている可能性があります。落ち着いてAngular-wayで進められないか考えたほうが、他の開発者へ実装の意図を示す上でも望ましいでしょう。

```ts
@Component({
  selector: 'my-foo',
  template: ``,
})
export class Foo {
  @Output() ev = new EventEmitter()
  ngOnInit() {
    this.ev.emit('hello')
  }
}

@Component({
  selector: 'my-app',
  template: `
    <my-foo (ev)="handle($event)"></my-foo>
  `,
})
export class App { 
  handle(ev: string) {
    console.log(ev) // hello
  }
}
```

Angularに慣れている方からすれば「そんなばかな」と思うかもしれませんが、実際に起こったケースです。

https://plnkr.co/edit/X1rrqoMLy54z2jn4lslw


## ViewChildで子Componentのメソッドを直接呼ぶ
### 問題点
`@ViewChild()`を使うことで、親Componentは子Componentのインスタンス参照を得られます。これによって、子Componentが持つメソッドを直接呼ぶことができます。

```ts
@Component({
  selector: 'my-bar',
  template: ``,
})
export class Bar {
  someMethod() {
    console.log('hello')
  }
}

@Component({
  selector: 'my-foo',
  template: `<my-bar></my-bar>`,
})
export class Foo {
  @ViewChild(Bar) barRef: Bar
  
  ngOnInit() {
    this.barRef.someMethod() // hello
  }
}
```
これを行うと、せっかくのコンポーネント指向による疎な実装から、一気に結合度が上がってしまいます。親子は常にペアであることが必須となってしまいます。たとえ呼べるからといって、子Componentのメソッドを無闇に呼ぶのはやめましょう。

https://plnkr.co/edit/tv2QR1Y5BrbRwZwGi6IR

### 解決策
そもそも行わないことが第一ですが、どうしてもそういった実装が思いついてしまう場合、その処理は実は親に書くべきではないかと検討し直したり、代わりに`@Output`で実現できないかを考え直しましょう。

筆者のケースで実際どうしようもなかったのは、子Componentの`<input>`に対する`focus`を当てる操作で、これは泣く泣くコメント付きで許容しました。

設計段階に立ち返ればもっとスマートな解決法がある気もしますが、やむを得ずこういった状況になった場合、作り直すための工数などから勘案して、何が一番要件を満たしつつ安全になるか検討するとよいです。

## ChangeDetectionStrategyを常に書かない
### 問題点
[`ChangeDetectionStrategy`](https://angular.io/docs/ts/latest/api/core/index/ChangeDetectionStrategy-enum.html)とは、そのComponentのchange detector（変更検知）を一度きりにするか、常時にするか指定するためのAPIです。

```ts
@Component({
  selector: 'my-app',
  template: `<my-foo></my-foo>`,
  changeDetection: ChangeDetectionStrategy.OnPush
})
export class App {
}
```

このように記述します。`ChangeDetectionStrategy.OnPush`は一度きり、`ChangeDetectionStrategy.Default`は常時でこれが初期値です。記述しない場合は`Default`が適用されますが、これは常に記述することをお勧めします。

### なぜ
開発初期には、書かないまま進めることが多いと想像します。ただ、パフォーマンス・チューニングの段階で`ChangeDetectionStrategy.OnPush`を書き始めたときに問題が発生します。

`ChangeDetectionStrategy.OnPush`を付けてみたらバグが起こったとしましょう。このとき`OnPush`を除去する修正はせずに、`Default`に書き換えるべきです。開発中期〜後期であとから`OnPush`を付けて回るとき、どのComponentにバグが起こったか分からなくなってしまうためです。

修正時に常に`Default`を付けるようにすれば、次の3ケースの判別が付きやすくなります。

- `changeDetection`が未定義のとき
  - 検証前
- `OnPush`のとき
  - 検証し問題がなかった
- `Default`のとき
  - 検証の上問題があったため`OnPush`は不採用

### 解決策
開発中期〜後期に`changeDetection`を意識し始めた場合は上記のようにすればよいでしょう。ではこの情報を開発前から知っていた場合はというと、これは最初から`ChangeDetectionStrategy.OnPush`を意図的に書いておくとよいです。

こうすることで「開発中に想定した動きをしなかった→`Default`に変更することで動くようになる」という実装フローになり、あとからのパフォーマンス・チューニング時に何度も`OnPush`と`Default`を切り替えながらデバッグする手間を省けます。

ただし、これは実装着手時に`OnPush`である前提のため、かえって分かりにくいと感じる方もおられるかもしれません。パフォーマンス要件が緩い場合は、これを一切気にせず常に`Default`でもいいでしょう。これはパフォーマンス・チューニング時の煩雑だった経験による提案です。

## `*ngIf`と`*ngFor`を一つのタグに書く
### 問題点
一つのタグ、Componentに対して`*ngIf`と`*ngFor`の両方を書いてはいけません。

```ts
@Component({
  selector: 'my-app',
  template: `
    <p *ngIf="state" *ngFor="let v of values">{{v}}</p>
  `,
})
export class App {
  state: boolean
  values: number[]
  ngOnInit() {
    this.state = true
    this.values = [0,1,2]
  }
}
```

これは例外が出るのですぐに気付いて修正できますが、慣れない間はやりがちです。

### 解決策
次のように入れ子にします。

```ts
@Component({
  selector: 'my-app',
  template: `
    <ng-container *ngIf="state">
      <p *ngFor="let v of values">{{v}}</p>
    </ng-container>
  `,
})
export class App {
  state: boolean
  values: number[]
  ngOnInit() {
    this.state = true
    this.values = [0,1,2]
  }
}
```

`<div *ngIf="">`と書きそうになりますが、ここでは`<ng-container>`をお勧めします。`<ng-container>`はテンプレート記述上では入れ子を宣言できますが、実際のDOM生成では無視されるため無駄な`<div>`を吐かずに済むという利点があります。

## いくつかのタグをまとめて`*ngFor`したいとき`<template>`でまとめる
### 問題点
次のようなマークアップを`*ngFor`で繰り返したいとき、どうすればよいでしょう。

```html
<dl>
  <dt>定義</dt>
  <dd>内容</dd>
</dl>
```

```html
<dl *ngFor="...">
  <dt>定義</dt>
  <dd>内容</dd>
</dl>
```

これだと`<dl>`がいくつも生成されるので誤りです。こういった`<dt><dd>`を常にペアで繰り返したい場合、検索してStack Overflowなどに辿り着いてしまうと、古い情報が案内されていることがあります。

```html
<dl>
  <template ngFor let-v [ngForOf]="values" >
    <dt>{{v.term}}</dt>
    <dd>{{v.description}}</dd>
  </template>
</dl>
```

例えばこのようなもので、`<template>`を使うものです。これはお勧めできません。

### 解決策
これも前節と同じように`<ng-container>`を使ったほうが安全です。

```html
<dl>
  <ng-container *ngFor="let v of values">
    <dt>{{v.term}}</dt>
    <dd>{{v.description}}</dd>
  </ng-container>
</dl>
```

`<template>`と`<ng-container>`の挙動は2.xの時点では同じですが、今後`<template>`を`<ng-template>`に[変更しようという動きが観察されます](https://github.com/angular/angular/issues/11994)。まだはっきりと公式が非推奨と謳っているわけではありませんが、同様の挙動であれば少しでも安全な方を採用したほうがよいでしょう。

## CSSに`:host-context`や`>>>`を使う
### 問題点
`:host-context(.foo)`セレクタは、自身のhost階層を遡って`foo`というclassが付与された親がいるかどうかを探します。自分の直近でない親まで辿ってしまい依存することになるので、とても変更に弱い指定となります。

また、`>>>` (piercing operator)も、自身より下層のScoped CSSにアクセスする機能なので過度に依存が増えてしまい、折角のコンポーネント指向を台無しにします。

### 解決策
使わないことが第一です。なによりコンポーネント指向としてそれぞれを独立して組むことがベストのため、スタイル周りで祖先や子孫に過干渉したくなる場合は構造を見直すのがよいです。せっかくのScopedなCSSなんですから、スコープを最小限にするよう心がけましょう。

AngularにおけるCSSのベストプラクティスは筆者自身もまだ模索中なので、今後のコミュニティからの知見に期待しています。

## 既存のPipeを活用した新たなPipeを定義する際に既存PipeをDIする
### 問題点
例えば、byteを受け取って、それを"100KB"や"1.23MB"など上手いこと返す`FilesizePipe`を作りたいとしましょう。このPipeは数を扱うのでAngular標準の`DecimalPipe`を活用できると便利です。

このとき、いくつか失敗しやすいため解説します。

```ts
@Pipe({name: 'Filesize'})
export class FilesizePipe implements PipeTransform {

  constructor(private decimalPipe: DecimalPipe) {}

  transform(byteString: string): string {
    // 処理
  }

}
```

まずDIです。一見うまくいきそうですが、PipeはServiceではないためDIできません。


```ts
@Pipe({name: 'Filesize'})
export class FilesizePipe extends DecimalPipe implements PipeTransform {

  transform(byteString: string): string {
    // 処理
  }

}
```

次に継承、これも失敗します。なぜならスーパークラスに`@Pipe({name: 'Decimal'})`が指定されておりバッティングするためです。

### 解決策

```ts
import {Pipe, PipeTransform, Inject, LOCALE_ID} from "@angular/core"

@Pipe({name: 'Filesize'})
export class FilesizePipe implements PipeTransform {

  private decimalPipe: DecimalPipe

  constructor(@Inject(LOCALE_ID) locale: string) {
    this.decimalPipe = new DecimalPipe(locale)
  }

  transform(byteString: string): string {
    // 処理
  }

}
```

正解はこのようになります。Angularを書いていてあまり標準APIをnewすることはありませんが、Pipeの場合はこのように扱います。`locale`をDIして、それを与えてPipeのインスタンスを生成するのがミソですね。これで`DecimalPipe`の処理を活用した独自Pipeが定義できます。

モックテストが行えないという指摘があるかもしれませんが、Pipeは`transform()`が引数と戻り値を扱えて単体テストが書きやすいため、あまりこの状況でモックは使わないでいいという判断をしています。どうしてもモックが使いたい場合、PipeよりもServiceが適している可能性があります。

この節はあまりアンチパターンと呼べるものではないですが、筆者がとにかくハマったので掲載しておきます。

# Angular 2.xは親切
Angular 1の頃から長く触ってきた身としては、Angular 2.xはAngular 1に比べてとにかく親切になったという印象があります。ドキュメントは豊富だし、例外ログも充実しています。そのため、まずドキュメントに沿った使い方をしていれば、つまずくことは減らせます。まずはAngular-wayで開発を進めることを推奨します。

慣れてきて凝った使い方をし始めるときにはミスが増えがちです。そのAPIが何を意図して用意されたものなのか、もう一度見つめ直すのがよいでしょう。

Angular 2.xからはclassベースとなったため、開発中のアンチパターンはどちらかというとオブジェクト指向設計そのものの進め方であったり、根本的なアーキテクチャであったりに寄りがちです。そういったclassベースの開発のノウハウはAngularでも活かせますので、既に他の言語、フレームワークでの開発経験が豊富な方ならば、おおよそ何が問題となりうるか想像がつくでしょう。本稿もそういったアンチパターンは掲載しないようにしました。

また、AngularではRxJSを活用できるよう設計されているため、慣れた方はpub/subベースの設計を採り入れることもあるでしょうが、そういった書き方をする際のアンチパターンもAngularの本質から外れるため今回は割愛しています。RxJSについては機会があればまた。

# 結び
2016年最大のAngularニュースといえばAngular 2.0.0リリースですが、今やもう2.2.xです。安定したAngularでこれから開発に着手される皆さんは、ドキュメントを確認しながら、どうぞ安全運転で開発を進めてください。

---

本稿はng-japanの@laco0416, @Quramy, @teyoshに査読いただきました。ありがとうございます。