-
Notifications
You must be signed in to change notification settings - Fork 311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
EngineIdをユニークな型にする #1172
The head ref may contain hidden characters: "engineId\u306B\u30E6\u30CB\u30FC\u30AF\u306A\u578B\u3092\u3064\u3051\u308B"
EngineIdをユニークな型にする #1172
Conversation
export const engineIdSchema = z.string().uuid().brand<"EngineId">(); | ||
export type EngineId = z.infer<typeof engineIdSchema>; | ||
export const EngineId = (id: string): EngineId => engineIdSchema.parse(id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ここがEngineIdを型定義している場所です。
が入ったあとに修正してマージが良さそう? |
#1092 が入ったmainをマージしました・・・! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
とりあえず気になった個所としてはこんなものかなぁと
Mapの箇所はおいおい変えたいなぁと思う程度で今回変更は不要かなと思いますが、characterInfos辺りが意図通りなのかが気になりました
@@ -88,16 +89,19 @@ const engineInfos = computed( | |||
() => | |||
new Map( | |||
Object.entries(store.state.characterInfos).map( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
型見る感じ単純なstringとして扱ってそうに見えたけれども、ここの変更は意図通りですか?
あと実際に動かしていないのですが、symbolを付けたstring(すなわちEngineIdですが)を再度EngineIdコンストラクタみたいなのを通すとどうなるんですか?(って質問しようとしてコード見たらzodでstringとして扱ってみたいな感じだったから、単純なtoString()が呼ばれて再度symbol付け直す感じなのかな…
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
characterInfosのkeyがstringとして扱われてるのですよね
EngineIdがkeyなのであれば、これは型として表現した方がいいなと思ったのでした(ということで、types.tsの方の変更requestです)
コメントを付けた箇所は、よくわからないstringをEngineIdとして扱っているわけではなさそうなので、ここに関しては変更不要です(Object.entriesの弊害がまた出てしまったみたいな顔をしながら
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
あ、なるほどです!ほんとですね!!
stringに代入できてしまうんですね・・・。
EngineId
になってなかったRecordがいくつかあったので一緒に直してみました!!
レビューありがとうございます!!! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
大丈夫だと思うのでマージします!!
(大丈夫じゃなくても致命的なエラーにはならないはず)
内容
EngineIdをただのstringからユニークな型(brand型)に変更します。
これによりRecordのキーなどの変数が厳密になり、コーディングミスを失くしやすくなるはず・・・!
関連 Issue
その他
方針としては、string型になっていたところを全てEngineIdにしています。
またzod schemaが必要なとろろはengineIdSchemaを指定しています。
一部undefinableにする必要があり、throw Errorするロジック周りがたされていたりします。