Wantedlyでは今年に入って、Reactの導入や、webpackの導入によるビルドプロセスの変更、CoffeeScriptからJavaScriptへの変更など、フロントエンドの開発環境が大幅に変更されました。このあたりの話は高松の記事に詳しく書かれていますが、よりリッチなUIを効率的にチーム開発できるようになり、とてもいい変化でした。

コードの質にばらつきが

React導入時は、一つのチームだけReactの開発を行っていたのですが、複数チームが独立して開発するようになると、同じReactのコードでもチームごとに雰囲気の違うコードが出来上がっていました。

ここは括弧を付けるべきなのか、このインデントは好ましいのか、ここはSyntac Sugerを使ったほうがいいんじゃないかなど、疑問点がある度にチーム間で確認をとるのは大きなコストになるため、どこまで指摘するかはチームのレビュアーに依存していました。

また、どんどん新しい人がコードを書き始めるようになると、そのチームのルールを共通認識として持たせることも難しく、どんどんレビュアーの負担が大きくなっていきました。

スタイルガイドを作ろう！

この問題を解決するために、チーム間で合意をとったスタイルガイドを作成し、それに沿ったLintを導入することにしました。

スタイルガイドを決めてしまえば、チーム間・メンバー間での認識の違いを防げるし、Lintを導入することで、レビュアーの目に頼らない機械的な調整が可能になります。

もちろんスタイルガイドを0から作成する時間はないので、ベースとなるスタイルガイドを決め、そこからWantedlyに合った調整を行うという方針にしました。ベースにしたのは、JavaScriptのスタイルガイドで最もPopularな、Airbnbのスタイルガイドです。外に出しても恥ずかしくないコードを書くようにするために、広く使われているものをベースに選択しました。また、ReactやJSXといったWantedlyのスタックに対応したスタイルガイドも用意されていたことも選んだ一因です。

このAirbnbのスタイルガイドとWantedlyの現状のコードを照らし合わせて、認識が共通化できていないと思われる部分と洗い出し、チーム間・メンバー間で合意をとっていきました。細かい部分もありましたが、主な論点となった部分について紹介したいと思います。

スタイルガイドをちゃんと読み込んでいる人にとっては当たり前のことばかりかもしれませんが、メンバー間で不満のない共通認識とするために、改めてその理由をきちんと調べるように努めました。

セミコロン;

真っ先に問題になったのは、行末のセミコロンです。JavaScriptは多くの場合セミコロンを自動で挿入してくれるため、基本的にセミコロンを書かないスタイルになっていました。普段RubyやCoffeeScriptを中心に書いているメンバーであったこともあり、無意識的にカンマを省略していました。しかし、セミコロンを省略することは、予期しない実行結果になる可能性があるため、Airbnbスタイルガイドではセミコロンを必須にしていました。

ご存知の方も多いと思いますが、簡単な例を示します。

let x = 10
[1, 2, 3].map(a => a * x)

このコードは、配列`[1, 2, 3]`の各要素に10をかけることを意図して書いたコードですが、実際に実行した場合、`[1, 2, 3]`の部分が前列末尾の`10`に対するプロパティのアクセスだと認識し、undefinedを返し、エラーを吐きます。行末にセミコロンがあれば、正しく実行されます。

このようなコードが書かれる可能性は普通にあるため、予期しないバグを生み出さないために、セミコロンの省略を禁止することで合意しました。

let, const

再代入しない変数の宣言には、letではなくconstを使用する、というルールです。宣言された変数がそのブロック内で再代入される可能性があるのかどうかを分かりやすくなるため、コードの見通しがよくなります。実際に、Reactのrenderメソッドで出てくる変数宣言は、ほとんどconstに置き換わります。

const { items, onClick } = this.props

" " vs ' '

文字列リテラルとして、シングルクォート、ダブルクォートどちらを使用するべきかという話があります。

Airbnbのスタイルガイドではシングルクォートのみを使うことが記されています。JavaScriptは、Rubyなどと違い、シングルクォートとダブルクォートに機能の差はないのですが、コード内の文字列を読みにくくする原因の一つであるバックスラッシュによるエスケープを最小限にするとの理由で、シングルクォートを推奨しています。HTMLを操作することが多いJavaScriptでは、`href="//www.wantedly.com"`などのHTML Attributesを文字列として扱う場合に、ダブルクォートのエスケープが必要にならない方がいい、という理由みたいですが、Reactを中心に開発している場合、そのようなエスケープが必要になる場面はほとんどありませんでした。

また、JSXはHTMLに似ているので、プロパティには逆にダブルクォートを使うというルールもありました。しかし、JSXがただのHTMLだと勘違いする人が多いため、あえてJavaScriptに合わせてシングルクォートを使うべきという意見もあり、あまりベストプラクティスとは言えない状況だと判断しました。

このような理由と、WantedlyではRubyでもクォーテーションを統一しない方針（同一ファイル内では統一する）なのも含めて、JavaScriptのクォーテーションは統一しない方針にしました。

case文のブロック

JavaScriptのcase文はブロックを作らないため、同じswitch中の複数のcase文で同じ名前の変数を宣言するとエラーになります。case文は、ReduxのReducerのコードでよく使用するため、度々この挙動に困ることがありました。

switch (action.type) {
case UPDATE:
  let newPost = action.post.merge(action.params);
  return state.set('post', newPost);
case PUBLISH:
  let newPost = state.post.publish();
  return state.set('post', newPost);
}

上のコードは、UPDATEとPUBLISHの個所で同じ名前のnewPost変数を宣言しているため、エラーが発生します。caseはブロックを作らないことに気づくと理解できる挙動なのですが、はじめによくハマるし、変数名が被らないようにするのも面倒です。

case文の中をblockで囲むようにすれば、この問題は解決できるので、case文は必ずブロックにするというルールにすることにしました。上のコードは以下のようになります。

switch (action.type) {
  case UPDATE: {
    let newPost = action.post.merge(action.params);
    return state.set('post', newPost);
  }
  case PUBLISH: {
    let newPost = state.post.publish();
    return state.set('post', newPost);
  }
}

caseをブロックで囲むようにすると、switchのブロックと最後のcaseのブロックの閉じタグが同じインデントで被るため、caseのインデントを一つ下げるようにしました。

Accessorは使わない

JavaScriptには、あまり使われいない印象がありますが、Accessorがあります。あるプロパティをget/setする際に、任意のgetter/setterメソッドを呼び出すことが出来ます。非常に便利な機能なのですが、JavaScriptに慣れた人として予期せぬ挙動になることが増えるなどの理由で、使わないという方針にしました。

予期せぬ挙動の一つとして、以下に示したコードのように、fooオブジェクトのbarプロパティをbazにsetしたのに、比較してみると同値ではなくなる可能性が出てくるというものがあります。

foo.bar = baz
if (foo.bar === baz) {
  // ここを必ず通るように見える
}

このような挙動は、Rubyなどの言語ではそこまで不自然ではない（メッセージパッシングによるメソッドの呼び出しであることを認識しているため）ですが、JavaScriptに慣れ親しんだ人からすると、ただオブジェクトのプロパティをget/setしているだけに見えるため、このような挙動は理解しがたいものです。

このような状況は、モダンなJavaScriptが広く浸透し、Accessorを使うライブラリなどが増えてくると改善されるかもしれませんが、現段階では混乱を生むと考え、Wantedlyでは使用しないという選択をしました。

拡張子.jsx

もっとも大きな変更になったのは、JSXが書かれたファイルは拡張子を.jsxにする、というルールです。.jsファイルは、はECMAScriptとして（将来的に）正しいコードであるべきで、JavaScriptエンジンが直接理解できるようなファイルであるべきです。この点で、JSXはJavaScriptの拡張であるため、正しいECMAScriptとは言えないため、.jsx拡張子のファイルにするべきです。Wantedlyでは、すべてのファイルを.jsで書いていたため、componentをまとめてあるフォルダをファイルをrenameすることにしました。

また、.jsxファイルをimportする際、webpackではデフォルトで`.jsx`を省略できません。省略することのリスクといえば、同じbasenameで拡張子だけ違うファイルがあったときにどっちをimportしたいのか分かりにくいことが考えられますが、.cssなどとは違い、.jsxと.jsが被ることは考えられないため、また既存のコードを変更する手間が大きいため、import時に.jsxは省略して書くというルールにし、webpackの設定を変更しました。

Ref callback

Reactのnode参照するためのrefプロパティは、文字列として参照名を指定するか、callback関数を指定することができます。Reactの公式ドキュメントやAirbnbのスタイルガイドでは、後者を使うように推奨しています。

前者の文字列での指定の方が、ただnodeを参照したいだけのときにはシンプルで簡単なのですが、この書き方ではできないことがあります。例えば、モーダルダイアログを表示した際に、bodyにclassを追加してスクロールを禁止したい、みたいな場面があるかと思います。

<Modal isOpen={this.state.isOpen}>
  <div
    ref={node => {
      this.node = node;
      if (node) {
        document.body.classList.add('fixed');
      } else {
        document.body.classList.remove('fixed');
      }
    }}
  >
    ...
  </div>
</Modal>

このような場合に、refのcallbackを使うことで、nodeのmount時に必ず呼ばれる処理を書くことができます。この例のようなシンプルな場合は、this.state.isOpenを更新する際のcallbackとして書くこともできますが、表示条件が複雑になった場合や、propsに依存する場合などは、refのcallbackで書く方が綺麗で安全なコードになります。

このように、callback形式でしか実現できない処理があるが、文字列での指定でしか実現できないことはないため、はじめからcallback形式で書くことが推奨されます。後で変更すればいいやと思っても、文字列の場合は this.refs.node でアクセスし、callbackの場合 this.node（実装者の自由ですが）というアクセス方法になるため、参照しているすべての個所で変更が必要になるため、はじめからcallbackで指定しておくのが最適です。

.bind(this)

JSXのcallbackに、そのコンポーネントのインスタンスメソッドを受け渡すとき、bind(this)した関数を渡す必要があります。その際に、renderメソッドのなかでbindしてしまうと、renderが走るごとにbindされた関数を生成してしまうため、パフォーマンスへ悪影響を与えることがあります。

例えば、Componentの無駄なrenderを避けるため、shouldComponentUpdateメソッドで前後のpropsの値を比較することはよく行われますが、その中でcallback関数が変更されいないか比較していた場合、bind(this)が行われる毎に新しい関数オブジェクトが生成されるため、this.props.onClick !== nextProps.onClick が常にtrueを返すようになるため、本当は変更されていないにも関わらずrenderが走ってしまうことになります。

このような状況を避けるために、bind(this)は、コンストラクタ内で行うようにすべきです。

constructor(props) {
  super(props);

  this.onClick = this.onClick.bind(this);
}

Async Function

Wantedlyでは、webpack(babel導入時からAsync functionを使用するようにしていました。今までcallbackやPromiseで処理していた非同期処理も、async/awaitを使用すれば同期的に書くことができ、コードの見通しがよくなります。

Async Functionの仕様は、先日stage-4に上がったため、ECMAScriptに正式に導入されることが決定しているため、これからはasync/awaitを使う書き方が主流になると考え、出来る限りAsync Functionを使うという方針にしています。

続きは次回

以上のステップで、メンバー間での認識のズレがない、JavaScriptスタイルガイドができました。これから、この設定を徹底するためにLintを導入したり、既存のコードをどうやってRefactorしていくか、などのステップがありますが、長くなりましたので、今回はここまでにしておきます。フォローやRSSなどで、Wantedly Engineer Blogの更新を追いかけてみてもらえると嬉しいです。

まとめ

・チームが大きくなり、コードを書くメンバーが増えるにつれて、コードのばらつきは大きくなり、見通しが悪くなり、レビュアーの負担が増え、生産性が下がる。

・スタイルガイドに従うことで、起こりやすい問題や、まだ起きていない潜在的な問題を防ぐことができる。

・スタイルガイドをただ盲目的に守るだけでなく、その理由についてきちんと調べ、チームの状況によって適切に扱うことが重要。