pr to leave comments #1
Conversation
- Replace {} type with Record<string, never> in global.d.ts
- Add type="button" to counter button for accessibility
- Fix unused variable error by prefixing with underscore
- Replace non-null assertions with default values using ||
- Convert imports to import type for better tree-shaking
- Use Number.isNaN instead of global isNaN for safety
- Organize imports according to project standards
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <[email protected]>
| updatedAt: text().notNull().default(sql`(CURRENT_TIMESTAMP)`), | ||
| }; | ||
|
|
||
| export const users = sqliteTable('users', { |
There was a problem hiding this comment.
AI に聞いた感じだとファイルを分けられるみたいなので、集約の単位でファイルを分割できると見通しがよくなりそうです。
https://claude.ai/share/289433f6-b76c-4502-8ca7-75a52b49e8f7
ちゃんとソースの確認まではできてないですが、流石にできるはず…
There was a problem hiding this comment.
ディレクトリ以下を全部スキーマとして扱うみたいなのができたはずです！
| import type { Ordinal } from '../values/ordinal'; | ||
| import type { PrizeSnapshot } from '../values/prizeSnapshot'; | ||
|
|
||
| export class Andon { |
There was a problem hiding this comment.
いくつか理由はあるのですが、データ型を定義する目的で class を使うのは避けたいです！ method の集合を定義する際の実装のパターンとしてだったり、小さな状態を持った特定の目的の utility の実現方法としてであれば class を使っても良いと考えていますが、それ以外では class を使うことが実装上のノイズとして作用してしまうことが多いだろうと考えています。
クラスの初期化方法が不便か、冗長になりがち
今書いてみていただいている方法だと、順番のある引数として各プロパティを定義していますが、この場合途中のフィールドを optional で定義するのが大変だったり、新しいプロパティを追加したときの実装の修正箇所が増えたりします。
destructured assignment を前提として定義することもできますが、それはそれで冗長な書き方になったりします。
例:
constructor({
id,
festivalNumber,
grade,
classNumber,
title,
description,
prizes,
}: {
id: number,
festivalNumber: Ordinal,
grade: number,
classNumber: number,
title: string,
description: string,
prizes: PrizeSnapshot[],
}) {
this.id = id;
this.festivalNumber = festivalNumber;
this.grade = grade;
this.classNumber = classNumber;
this.title = title;
this.description = description;
this.prizes = prizes;
}「クラスのインスタンスである」という情報のあるなしをロジックに持ち込みたくない
TypeScript （JavaScript） のクラスはどうあがいてもオブジェクトの延長線上でしかないため、データがこの形であり続けることの保証がありません。にも関わらず instanceof などを使うことで、そのデータ型であるかどうかの判定っぽいことができてしまいます。この辺は TypeScript の言語サポートによっていい感じに型を絞り込むためにも使えたりするのですが、 { ...data } みたいな書き方をしたり、代入可否判定の振る舞いなどの関係で、状況によってはあまり信頼性が高くありません。 TypeScript を正しく使えれば instanceof などに頼らずとも型安全に実装ができるので、変に型を絞り込めそうな雰囲気をだしてしまう instanceof を使える状況に持ち込まない方向性でいきたいです！（逆に言えば instanceof の信頼性を落とす部分を解決できれば良いという話でもあるので、そういうアプローチもできますが、このプロジェクトでそれをやるのは too much なためやめておきたいです。）
ちなみに instanceof は基本的にはエラーの識別など、自分たちでインスタンス化をしない（する場合でも 100% パターン化できる）ものに限定すれば割と使います。（というか throw で型情報が失われるので instanceof 以外がしんどい）
多分他にもありそうですが、主要なところだとこんな感じかなと思います。
There was a problem hiding this comment.
じゃあどうするか、みたいな部分は追って自分のほうで提案をできたらなと思っていますが、時間の確保的に次回の会議までになんとかできるかどうか…という感じになるかもです 🙏
There was a problem hiding this comment.
ありがとうございます！
そういう視点助かります🙏
entities だけではなく他も class やめた方がいいですかね？全部 class になっちゃってるので
時間の確保的に次回の会議までになんとかできるかどうか…という感じになるかもです 🙏
了解です忙しいところありがとう🙏
There was a problem hiding this comment.
repository とかの、メソッド（＋あっても定数）の集合の表現であれば class でも問題ないと思います！それらに関しては依存の注入とかをする場合に class だったほうが見通しが良くなったり、 class であることのつらみをあんまり受けないので、むしろ class のほうが良いかも？という気はしています。（もちろん class じゃなくてもかけますが…！）
| let festivalNumber: number; | ||
| try { | ||
| festivalNumber = Ordinal.parse(festivalOrdinal).inner; | ||
| } catch (_error) { |
There was a problem hiding this comment.
WIP: TypeScript の try-catch は絶望的に厳しい部分があるので、エラーハンドリングについてはいい感じの方法を定めたい
There was a problem hiding this comment.
エラーが型に出てこないの怖すぎて Result 的なやつ使いたいなーと思っていたのですが
https://qiita.com/devneko/items/48b0f438f7b48991a08b
こういう意見もあり躊躇していました！
いい感じなやり方あれば教えていただきたいです🙏
There was a problem hiding this comment.
try-catch から逃れられないというのは正しいけど正しくない、という感じですね…！
まずは一旦外部パッケージなどのことを忘れて考えると、基本的に自分たちで書けるコードの範囲については、型安全に書いていれば変なタイミングで throw されることはないと考えて大丈夫です。それでも throw されてしまったエラーは実装の考慮漏れなどの unkown-uknown で、これは API 実装の根本で try-catch して拾ってエラーレスポンスを返すようにするのが良いと思っています。
次に、外部パッケージのことを考えると、これは徹底的に自分たちで wrap して Result 型的なものにしてから使うようにすることで（特に外部プロセスとの通信を伴うものに関しては必須にしたい）、 try-catch の使用箇所を最低限に抑えることができます。これ系の try-catch は1回ちゃんと書いておければあとは型安全に処理を書けるので、まぁ受け入れられるかなと思います。
この2つの前提のもとであれば、自分たちが書くドメインロジック上で try-catch をすることはほぼなくなるはずですが、 ドメインロジック上で考慮したくないものについては throw して、根本の try-catch にまかせてしまう場合もあります。考慮したくないものの例としては、考慮するとあまりに冗長になるものや、ほぼ発生しないエッジケースなどです。
| await festivalRepository.findFestivalByNumber(festivalNumber); | ||
|
|
||
| return c.render( | ||
| <div class="py-8 text-center"> |
There was a problem hiding this comment.
WIP: 普段バックエンドから直に HTML 返す系のことをしないので、何が最適かはわかっていないが、分離したい気持ちがある
No description provided.