はじめに
前回の記事 では駆け出しrailsエンジニアとして受けたレビューを紹介しました。
フロントエンドの実装にも首を突っ込む機会があったので、今回はjs(厳密に言うとtypescriptですが)の実装で受けたコードレビューを紹介しようと思います。
晒してばっかりですが、駆け出しのうちは恥をさらしてなんぼではないでしょうか。
いってみよう。
マークアップとの依存を低くすることを意識しよう
parentElement等のhtml構造に大きく依存するDOM取得方法は避けること。
html構造に依存している=htmlの変更でjsがすぐバグる
ということなので、マークアップエンジニアやコーダーの人がjsの実装を気にしなければいけなくなってしまう。
例: ボタンのクリックイベントでそのボタンが属する<tr>をとってきたいとき
<table>
<tr>
<td><button id="ng-btn" type="button">NG</button></td>
</tr>
...
</table>
<script>
const $button = document.querySelector("#ng-btn");
$button.addEventListner("click", function(){
<!-- このとり方だと<td>直下がボタンじゃなくなったら動かなくなる -->
const $tr = $button.parentElement.parentElement;
...
});)
</script>
<table>
<tr class="hoge-row">
<td><button id="ok-btn" type="button" data-raw-class='hoge-row'>OK</button></td>
</tr>
...
</table>
<script>
const $button = document.querySelector("#ok-btn");
$button.addEventListner("click", function(){
const $tr = document.querySelector(`.${$button.dataset.rawClass}`);
...
});)
</script>
だいぶ雑な例示ですが、大雑把な方針としては、
- 子→親の直接の取得はhtml構造に必ず依存するので避ける
- 子には親に関する情報をもたせた上で、その情報から改めて親を取得する
jsにstyleの情報を持たないようにしよう
styleの管理はcssにまとめたほうが煩雑にならない。
jsではclassの付け替えをする等にとどめておき、cssでそのclassにたいしてスタイルを振れば良い。
例: 動的にdisplay:noneつけたいとき
<div id="ng">ng</div>
<script>
const $div = document.querySelector("#ng");
$div.style.display="none"
</script>
<style type="text/css">
.is-hidden {display:none;}
</style>
<div id="ok">ok</div>
<script>
const $div = document.querySelector("#ok");
$div.classList.add('is-hidden')
</script>
パフォーマンスを意識できるようになろう
addEventListenerを使うときは無名関数は使わない
addEventListenerを使うときは無名関数は使わないようにする。
新しくfunctionを切ってそれを呼ぶようにする。
メリットは大きく以下
-
そのlistenerを使い回せる
→ 他のeventにも使える -
eventを削除できるようになる
→ 不要なeventが残ってしまうのは健全ではない。無名関数だとremoveするのに苦労する。 -
メモリ的にもやさしい
→ 無名関数だと呼ばれるたびにメモリを新たに消費するが、functionとして切り出してあればキャッシュされる。
例:あるボタンへのクリックイベント
$button.addEventListner("click", function(e){
console.log(e.target)
...
});
$button.addEventListner("click", _handleBtnClick);
function _handleBtnClick(e){
console.log(e.target)
...
}
appendChildは毎回リフロー・リペイントされるから工夫して使う
そもそもリフロー・リペイントとは。
リフロー ・・・ 各HTML要素を解釈して、それぞれが占めるスペースを算出する
リペイント ・・・ 算出したスペースのなかに、内容を描画していく
そしてリフロー・リペイントは以下をトリガーとしておきる。
- DOM ノードの追加、削除、更新
- display: none （リフローとリペイント）、あるいは visibility: hidden （位置の変更は起きないので、リペイントのみ） による DOM ノードの見た目の変更
- ページ中の DOM ノードの位置の移動やアニメーション
- スタイル属性のちょっとした変更のためのスタイルシート追加
- windowサイズの変更やフォントサイズの変更、そしてスクロールなどの、ユーザーの操作
appendChildはもろにDOMノードの追加にあたるので、連続で呼ぶのはよろしくない。
こういうときは、createDocumentFragmentを使おう。
描画されているDOMにappendされるまではリフロー・リペイントされない。
例:trタグにtdタグを複数appendしたいとき
$tr.appendChild($td1);
$tr.appendChild($td2);
$tr.appendChild($td3);
const $fragment = document.createDocumentFragment();
$fragment.appendChild($td1);
$fragment.appendChild($td2);
$fragment.appendChild($td3);
$tr.appendChild($fragment);
NGの例では3回もDOMノードの更新が走るが、OKの例では1度で済む
最後に
こういうポイントも意識するべきなどありましたら、ぜひご指摘のほどよろしくお願いします。