Skip to content
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

PresetKeyに型を付ける #1217

Merged
merged 7 commits into from
Mar 1, 2023

Conversation

Hiroshiba
Copy link
Member

内容

PresetKeyをブランド型にしました。
EngineIdやSpeakerIdやAudioKeyと全く同じやり方です。

ちょっと不思議な点があったのでWIPです。

関連 Issue

close #1210

その他

なぜかBrand型をkeyにしたRecordがPartialになる(valueがoptinalになる)ので、zod側にissueを立ててみました。

Comment on lines +19 to +21
export const presetKeySchema = z.string().uuid().brand<"PresetKey">();
export type PresetKey = z.infer<typeof presetKeySchema>;
export const PresetKey = (id: string): PresetKey => presetKeySchema.parse(id);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ここが型定義している場所です

@k-chop
Copy link
Contributor

k-chop commented Feb 20, 2023

なぜかBrand型をkeyにしたRecordがPartialになる

#956VoiceId をBrand型にしたときにハマって調べたんですが、現状利用側で as して回避するしかなさそうでした。

流れとしては
このissue↓の対応で、 z.record() は全部 Partial<Record<_, _>> にマッピングされるようになり

その後↓で z.record(z.string(), z.string()) 等のキーがprimitiveな場合のみ Partial が付かなくなったようですが、Brand型は対応されていなさそう、という感じです

@Hiroshiba
Copy link
Member Author

Hiroshiba commented Feb 20, 2023

おおお、ありがとうございます!!!

うーん、なるほど。。
いろいろ試そうとしてみましたが、asしかない気がしますね・・・ 😇
例外的にasにしたいと思います 😇

@Hiroshiba
Copy link
Member Author

Hiroshiba commented Feb 23, 2023

@k-chop さん
すみません、全然こちらのPRに関係ないのですが、ちょっと型パズルの相談が。。

2023/02/24 7:31追記 良さそうなのができたのでたぶん解決しました! ので書いていたのはいったん閉じます。

経緯

zodの方にいろいろ提案してみて、Partial化されるところのコードを教えてもらいました。

https://github.com/colinhacks/zod/blob/981af6503ee1be530fe525ac77ba95e1904ce24a/src/types.ts#L3061-L3069

stringもsymbolもPartialじゃないRecordになってるっぽいので、じゃあBRAND型もPartial型じゃなくて良いのでは的な提案をしたいな~と。
そのための型パズルがわからず。。。。

↓でコメントを書いてるとこが本題箇所です

import { z } from "zod";

export const presetKeySchema = z.string().uuid().brand<"PresetKey">();
export type PresetKey = z.infer<typeof presetKeySchema>;

type RecordType<K extends string | number | symbol, V> = [string] extends [K]
  ? Record<K, V>
  : [number] extends [K]
  ? Record<K, V>
  : [symbol] extends [K]
  ? Record<K, V>
  : [string & z.BRAND<"PresetKey">] extends [K] // ここを任意のz.BRANDに対応させたい。。
  ? Record<K, V>
  : Partial<Record<K, V>>;

type Hoge = RecordType<PresetKey, string>; // PartialじゃないRecordになってる

2023/02/24 2:21追記
これでいけそうと @y-chan さんに教えていただきました!!

type RecordType<K extends string | number | symbol, V> = [string] extends [K]
  ? Record<K, V>
  : [number] extends [K]
  ? Record<K, V>
  : [symbol] extends [K]
  ? Record<K, V>
  : K extends z.BRAND<infer B> // changed point
  ? Record<K, V>
  : Partial<Record<K, V>>;

@Hiroshiba Hiroshiba marked this pull request as ready for review February 23, 2023 22:25
@Hiroshiba Hiroshiba requested a review from a team as a code owner February 23, 2023 22:26
@Hiroshiba Hiroshiba requested review from y-chan and removed request for a team February 23, 2023 22:26
Comment on lines 828 to 834
// プリセットの変更
const changePreset = (
presetOrPresetKey: PresetSelectModelType | string
): void => {
const presetKey =
typeof presetOrPresetKey === "string"
? presetOrPresetKey
: presetOrPresetKey.key;
const changePreset = (presetKey: PresetKey | undefined): void => {
store.dispatch("COMMAND_SET_AUDIO_PRESET", {
audioKey: props.activeAudioKey,
presetKey,
});
};
Copy link
Member Author

@Hiroshiba Hiroshiba Feb 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typeofで切り分けるのが危なそうだったのでロジックを変えています。
といっても、PresetSelectModelType型のときはその.keyPresetKeyなのでかなり簡単でした。

Copy link
Member Author

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

できたのでWIPあけました!

z.BRAND型のRecordはPartialになる仕様なのでasで型を変換というコメントを書いていますが、これは変更できないかをzod側にPR作っています。

@Hiroshiba Hiroshiba changed the title WIP: PresetKeyに型を付ける PresetKeyに型を付ける Feb 25, 2023
@Hiroshiba
Copy link
Member Author

すみません! これ見ていただけると・・・! @y-chan

Copy link
Member

@y-chan y-chan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

おおよそLGTMですが、zodに対するPRがマージされているのであれば、一旦コミットを指定してzodをアップデートし、asを外すのもいいかな...?と思いました(ただ、package.jsonにコメントが書けないので、なぜzodをnpmではなくgithub経由でインストールするようになっているかを表せない可能性がありますが...)

@Hiroshiba
Copy link
Member Author

Hiroshiba commented Mar 1, 2023

latestを取り込むのは基本的に危険だと思います 😇
我々のVOICEVOXのように・・・。

個人的には、まあやむを得ない場合のみにしときたいかもです。

@y-chan
Copy link
Member

y-chan commented Mar 1, 2023

であれば、asを使っているところにTODOコメントを残しておくなどはどうでしょうか?

TODO: 将来的にzodのバージョンを上げてasを消す
ref: https://github.com/colinhacks/zod/pull/2097

とか...?

@Hiroshiba
Copy link
Member Author

改良してみました! f3dc6b4

Copy link
Member

@y-chan y-chan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTMです!

@Hiroshiba
Copy link
Member Author

問題ないと思うのでマージします!!

@Hiroshiba Hiroshiba merged commit 82c074e into VOICEVOX:main Mar 1, 2023
@Hiroshiba Hiroshiba deleted the PresetKeyに型を付ける branch March 1, 2023 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AudioKey、PresetKeyにそれぞれにユニークな型を作る
3 participants