asyncを使っているのに生かしてなかったのを修正。型を追加

h-takeyeah commented 3 years ago

Promiseにまつわるエラーハンドリングのやり方を修正したのとそれに合わせて型を作りました．

models/以下のファイルに書かれた関数は基本的に0番目に結果，1番目にあればエラーを含む配列を返すようになっていました．これを結果だけを返し，mysqlがエラーをthrowしても放置するように変更しました．

これらの関数はもとより全てasync functionなのでmysqlがエラーをthrowするとreject()に流れます．それを今まで生かしてなかったので，今回，呼び出し側のcontrollers/以下のファイルに書かれた関数にtry catchを書いて例外処理をするように変更したというわけです．こうするとreturnに書かれるものが必然的にPromiseがfulfillされたときのものになり，エディタの補完が少し便利になります．

それからSQLのスキーマに対応するTSの型を作りました．models/側で返す前にSQLクエリを発行した結果をチェックした上でこの型で型アサーションして返すことで呼び出し側での補完が少し便利になります．

// model側
type InRoomUser = {
  'user_id': number,
  'entered_at': Date
}
async function getUser (userId: number): Promise<InRoomUser> {}

// controller側
try {
  // 今までany[]だったものがInRoomUserになるので便利
  const user = await table.getUser(parseInt(req.params.user_id))
  //...以下略
} catch (err) {
  // rejectされたらここに来る
  console.error('[!] Error', err)
}

レビューよろしくお願いします．

equal-l2 commented 3 years ago

自分は例外がメチャクチャ嫌いなので、個人的にはこの変更は受け入れ難いです。 (もちろん責任者はあなたなので、そう決めたなら従うまでですが……)

このPRが実現したい利益はなんですか？戻り値の型を明確にしたいのであればtuple typeや専用のオブジェクト型を作ることでも対応できます。

equal-l2 commented 3 years ago

全くの参考として出しておくと、自分が好んで使うRust言語では、この種の「エラーまたは値」という構造はResultと呼ばれるEnum型(TypeScriptで言うunion型?)で表現されます。 (TypeScriptのunion型はvariant判定が色々闇っぽいのでこのまま移すのは無理ですが)

h-takeyeah commented 3 years ago

なるほど．エラーハンドリングを上位に持ってくるのは確かに気持ち悪かったですね．その変更はやめますないことになりました．取り込みます

h-takeyeah commented 3 years ago

このPRが実現したいことはエラーハンドリングのやり方を変更することでした．Promiseを返してくれるのだからその例外処理はtry，catchに任せればいいし，正常系だけ考えれば良くなるし，行数も減って良いかなと思ったので．

型を作ったのはおまけですが，使えますかね？

equal-l2 commented 3 years ago

確かに今回の場合だとエラーが起こる可能性はかなり低いし異常が起こったら死ぬだけなのでこれでもいいかもしれないですね

equal-l2 commented 3 years ago

あれ、閉じちゃうんですか？話を聞いていたら結構悪くないなと言う気になってきていたんですが。

(もし続けたいのであればReopenしてください)

h-takeyeah commented 3 years ago

@equal-l2 これ以上やることはないのでレビューの上approveをお願いいたします．

su-its / ams-backend

asyncを使っているのに生かしてなかったのを修正。型を追加 #73