こんにちは、エンジニアの@sota1235です。
タイトルの通り、今回は愚直に改善をした話をします。
メルカリのJavaScript
メルカリにおけるJavaScriptの活用場面は以下のようなものがあります。
- メルカリWeb
- アプリ内Webview
- 社内ツール
- React Native
- Node.js製のbotやGoogle App Scripts
- etc…
いずれもサービスにとって重要なものであり、サーバサイドエンジニアであってもJavaScriptに触る機会は少なくありません。
かくいう私も普段はサーバサイドエンジニアですが、JavaScriptコードを書いたりレビューする場面が多くあります。
そんな中でWebチームにおいて、JavaScript開発でいくつか問題がありました。
課題その1: JavaScriptのレビューコスト問題
1つ目の課題としてJavaScriptのコードをレビューする際、本質的でない部分に時間を取られる問題がありました。
ここで言う本質的ではない、とはプロダクト目線でないという意味です。
メルカリWebはES5の記法とES2015+の記法が入り混じっており、以下のような指摘が多くありました。
var
でなくconst
,let
を使用してください- template literalだと簡潔に書けます
- Objectのkey/valueはshorthandで書けます
これらの指摘は新しい記法の共有やコード品質の向上という点では有益です。
しかしながら、サービスロジックや設計とは全く関係のない要素でもあります。
このようなものは機械に任せて、人間には人間にしかわからないサービスロジックや設計のレビューにもっと時間を割きたいと考えました。
課題その2: 不具合発見の難しさ
JavaScriptは動的型付け言語であり、コンパイル時に明らかなバグに気づくということはできません。
例えば以下のような単純な不具合を全て人力で察知するのは難しいです。
- 関数名のtypo
- 不必要な変数の上書き
- etc…
これらをなるべく導入コストの小さな仕組みで察知したいという願いがありました。
特にメルカリWebはコードが3つの国(JP, US, UK)にまたがっており、依存関係を全て把握するのが難しかったのでこのようなものは人間が全てチェックするよりも機械的にチェックをしたいと考えました。
対策の検討
これらの問題に対する解決策にはいくつか選択肢があります。
例えばFlowやTypeScriptといった型機能、E2Eテスト、ユニットテストの導入等です。
しかしそれらを導入するにはコードベースが巨大化しており、コストがかかると考えました。
そこで初めは小さく簡単に導入が可能なESLintで改善を試みることにしました。*1
ESLintの導入
ESLintの解説はこの記事では省略します。詳しくは公式ドキュメントをご覧ください。
ESLintの導入により、Pull RequestごとにCIでコーディングスタイルの誤りや最低限の不具合を静的解析で検知することができます。
もちろん、全ての要素を拾うことはできませんがこれをうまく導入できればレビュアーがより本質的なレビューに時間を割くことができるようになります。
何より小さく少しずつ導入することが可能です。
導入手順
ESLintの大きな特徴として何一つルールのない状態からコードチェックを始められる点があります。
そこでWebへのESLint導入は以下の手順を踏むことにしました。
- eslint-config-airbnbを導入する
- 導入後、定義されている全てのルールを上書きして無効化する
- 無効化したものから取捨選択を行い、少しずつルールを有効化。コード修正を行う
- ある程度修正できたところでQAを行いリリースする
順番に説明していきます。
eslint-config-airbnb
eslint-config-airbnbはAirbnbがOSSとして公開しているESLintのルールセットです。
メルカリWebではES2015+, Reactを導入していたので利用者も多く相性もよい上記packageを導入することしました。
まずはESLintを落とし、eslint-config-airbnbを適用し、これを既存コードに実行してみます。
盛大に怒られました本当にありがとうございました。
ルールを無効化する
最初から2000個以上のエラーを修正するのはいくらなんでも無謀ですし、レビューする側の負担も大きいので一度ルールを全て無効化することにしました。
ESLintでは設定済みのconfigを上書きすることができます(設定は全て後勝ちする)。
例えばeslint-config-airbnbではno-undefルールが定義されています。
これを上書きするにはリポジトリの.eslintrc
で同じルールを再定義します。
{ "extends": [ "airbnb" ], "rules": { "no-undef": "off" } }
これでno-undef
ルールはチェックされることがなくなります。これを全ルールに適用すればエラーが0件になります。
ルールを少しずつ有効にする
次にこの無効化した全ルールから少しずつルールを有効化し、コード修正を行います。
基本的な流れは以下の通り。
- ESLint適用の親ブランチでPRを立てる
- 親PRから子ブランチを切り、1~数個ルールを有効化、コード修正したPRを親ブランチに向けて投げる
- 子ブランチ単位でコードレビュー依頼
この流れを取ることによりコード差分を最小限に抑え、少しずつルール適用を進めていきました。
QAを行う
ある程度、親ブランチが大きくなった段階でリリースに向けて準備を行いました。
リリース時点で子ブランチは4つ、有効化したルールは41個でした。
ESLintの修正により、JavaScript変更の影響範囲は全ページに及んでいたので全機能、全Regionに対するQAを依頼しました。
(QAチームについてはこちら)
コードレビューでは拾いきれない部分やユーザ目線による機能チェックにより、安心した状態でリリースに望むことができました。
苦労したところ
各自のタスクの兼ね合いやリソースの問題で時々止まりつつも4ヶ月に渡り進めていたのですが、その間にmasterブランチの変更を追従していくのが非常に大変でした。
日々多くのリリースが行われていくので、ESLint対応完了→QA→QAフィードバックというサイクルを回す間に新たに要修正箇所が増えてQAコストが上がってしまうことがありました。
よかったところ
時間こそかかりましたがESLintの導入によって当初の狙い通り、効率的なレビューができるようになりました。
また、例えばno-undef
ルールで未定義変数の参照を検知する等、誰も気づくことのできなかったバグを意図せず発見することもでき、ESLintによる解析の価値を確信しながら進めることができました。
今回はAirbnbのOSSを利用しましたが、今後はメルカリのコーディングスタイルに合わせた独自のESLint configを作り、JavaScriptを利用する他プロジェクトでも利用していこうと計画しています。
最後に
メルカリでは効率的なJavaScript開発を支えるフロントエンドエンジニアを待っております。
*1:Flowも小さく導入することができますが学習コスト等を考慮し、まずESLintの導入を優先しました