Add: getOrThrow / deleteOrThrowを追加 #2055
Conversation
sevenc-nanashi
changed the title
There was a problem hiding this comment.
プルリクエストありがとうございます！！
なるほどglobalに書くことによって認識させるんですね！！
実装理由も書いてあるのめちゃくちゃいいですね。
それが解消されたら別の解決策が使えるかもしれない。
Mapにメソッドを破壊的に追加する場合、起こりそうな問題をちょっと列挙してみました。
- 全てのライブラリのMapがこうなるので謎のバグが発生するかもしれない
- 例えば
Map.getOrThrowというメソッドがなかったら処理を追加する子クラスを定義されてたりとか（なさそうだけど可能性としてはある）
- 例えば
- 初学者がこれをVOICEVOX独自のものだと認識できない
- ランタイム？が異なるとコンパイルは通るのにコードが動かないことがある
- 例えばelectronのメインプロセスからはメソッドが見えるけれども実行時エラーが出るはず
- 他にもWorkerなどでも同じことが起こるかも
これらの問題を低減する方法をいくつか考えたのでコメントしていきます 🙇
@yamachu すみません、ちょっと熟練者としてアドバイスをお願いできると 🙇
今回のプルリクエストのような形を実装するのってどう思われますか･･･？
多分本来はやっちゃいけないレベルなんだろうとは思うのですが、メリットも結構大きいのでとても迷ってます。
代替案としてはメソッドを諦めてgetOrThrowという関数を実装する形があります。
個人的には、globalではなく明示的にインポートすれば使える、という形にできればリスクも減らせてメリットが上回るのかなと考えてます。
できないなら･･････どっちだろう。。。
src/helpers/strictMap.ts
Outdated
| declare global { | ||
| interface Map<K, V> { | ||
| getOrThrow(key: K): V; | ||
| deleteOrThrow(key: K): void; | ||
| } | ||
| } |
There was a problem hiding this comment.
これが独自のものだと気づきやすいように、/** */でコメントを書いておくと良いかも
src/helpers/strictMap.ts
Outdated
| }, | ||
| ); | ||
|
|
||
| declare global { |
There was a problem hiding this comment.
ちょっと詳しくないのですが、このファイルをimportしたら見えるようにする事って可能だったりしますか･･･？
というのも現状でelectronのメインプロセスから使えないのに見えるようになってて問題だな～～と感じたためです。
（その場合はaddPropertyが何度も実行されることも考えないといけないかも？）
src/store/singing.ts
Outdated
| const phrase = state.phrases.get(phraseKey); | ||
| if (phrase == undefined) { | ||
| throw new Error("phrase is undefined."); | ||
| } | ||
| const phrase = state.phrases.getOrThrow(phraseKey); | ||
|
|
There was a problem hiding this comment.
念のために将来revertするかもと考えて、このコミットでの変更は最小限にしておくのはどうでしょう･･･？
src/components/Sing/SequencerPhraseIndicator.vueの１つだけにしておくとか。。
|
prototype汚染はあまり良い印象がないですねー… Hiroshibaさんが代替案として挙げている prototypeに対して影響を及ぼす方法は、そのモジュールをimportしただけでprototype汚染が走ってしまうため、予期しないタイミングで挙動が変わってしまうみたいなデメリットはあるのかなと。 もしもprototypeに影響を及ぼす方法を取るのであれば、将来的にMapに export const getOrThrow = Symbol();
Object.defineProperty(Map.prototype, getOrThrow, {
value: (key) => {
if (!map.has(key)) {
throw new Error('key not found');
}
return map.get(key);
},
writable: false,
enumerable: true,
configurable: false,
});
//
import { getOrThrow } from '@lib/StrictMap';
const a = new Map();
a[getOrThrow]("a") // throw new Error('key not found')...みたいにSymbolを使う形にするのであれば、まだいいかな…とは思いました。（それでもimport時に副作用が発生するので、prototype汚染を行う箇所をuseStrictMapみたいな名前の関数として切り出して、entrypointで呼び出す、みたいなののをして、アプリケーションの初期化の中に含めてVOICEVOXのアプリケーションの知識として加えるとかは必要になります） 他にも、 interface StrictMap<T, K> extends Map<T, K> {
getOrThrow: (key: string) => K;
}
export const createStrictMap = <T, K>(params): StrictMap<T, K> => {
const map = new Map<T, K>(params);
Object.defineProperty(map, 'getOrThrow', {
value: (key) => {
if (!map.has(key)) {
throw new Error('key not found');
}
return map.get(key);
},
writable: false,
enumerable: true,
configurable: false,
});
return map;
}
//
import { createStrictMap } from '@lib/StrictMap';
const a = createStrictMap(new Map()) // とか createStrictMap([['a', 1], ['b', 2]]);
a.getOrThrow('a'); // 1
a.getOrThrow('c'); // throw new Errorみたいに、 と、方法はいくつかありますが、各種引数の型などを変えずに使う側でthrowするかどうかを選べる関数としての提供がいいんじゃないかなぁとは思いました（型として提供してあれば、必ず値があるというのを明示できるのでそれはそれで良さがありますが） |
|
@yamachu ありがとうございます！！！ 暗黙的なimport順序というかルールが出来上がるというの、気づけてませんでした。。 discordでもいろいろ教えてもらいました 🙇 アイデアとリスクをまとめるとこうでした。
リスクとリターン考えると、一番バランス取れてるのはgetOrThrow関数なのかなと思いました 🙇 |
|
まぁgetOrThrowが1番丸いと思います。 |
sevenc-nanashi
changed the title
内容
MapにgetOrThrow / deleteOrThrowを追加します。
関連 Issue
（なし）
スクリーンショット・動画など
（なし）
その他
（なし）