-
Notifications
You must be signed in to change notification settings - Fork 288
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
applyPatchesを自前実装に置き換える #2052
base: main
Are you sure you want to change the base?
applyPatchesを自前実装に置き換える #2052
Conversation
This reverts commit c4f96ff.
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.
ちょっとお力お借りしたく思いアサインさせていただきます 🙇
@sevenc-nanashi @Segu-g
@White-Green 実装見ました! 1つ目はこのコードはImmerの 2つ目はカスタムクラスが入ってきた時にエラーが出ないことです。 以上、確認をよろしくお願いします! |
@Segu-g コメントありがとうございます
現状問題が想定できるのは独自classぐらいだと思うので、独自class非対応の旨のみコメントを入れておきました
こちらはどうしようか悩んでいるところです
このようなトレードオフに関してVOICEVOXでの方針などがありましたら教えていただけますでしょうか |
今気づいたのですが、このコードは
若しくは、このプロパティの利用箇所であるcommandが対象とするStoreに対して静的検査を用いて型に制約を掛けてしまうのもアリだと思います。structuredCloneの対応型を明示して列挙する必要がありますが、コードが対応している型を表現できるならフールプルーフ機構として動作するかも?型を工夫すれば |
これは純粋に見落としていました 独自classなどをcloneした際に検知するフールプルーフの機構ですが、とりあえず再帰的にすべてを確認する方針のものを実装してみました |
フールプルーフの実装ありがとうございます! 確かに実装の複雑さ(コードの量)が上がりますね.... 提案なのですが、現在の「 // null, undefinedの場合
if (value === null || value === undefined) {
return value;
}
// コンテナ型は走査し再帰的にclone
// plain Object (Record) の場合
const valuePrototype = Object.getPrototypeOf(value);
if (valuePrototype === null || valuePrototype === Object.prototype) { ... }
// Arrayの場合
if (Array.isArray(value)) { ... }
// Mapの場合
if (value instanceof Map) { ... }
// Setの場合
if (value instanceof Set) { ... }
// コンテナ型でない場合はcopyかstructuredClone
// function の場合
if (typeof value === "function") return value;
// それ以外の場合
try {
const clonedValue = structureClone(value);
assertPrototypeEq(value, clonedValue);
return clonedValue;
} catch {
...
} P.S. type T = { x: T }
const x: T = {} as T
x.x = x
console.log(structuredClone(x)) |
フールプルーフ機構も含めてコードが複雑になってしまったので、むしろ |
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.
@Segu-g さんありがとうございます!!
@White-Green
時間ができたので見させていただきました!
とても勉強になりました!!
現在は設計がある程度のパターン出てきたフェーズだと感じています。
あとちょっともしかしたら裾が当初の目的よりも広がっているのかもと感じました。
ということで別視点ですが、着地点をどこにするか相談させてください!
個人的に、このプルリクエストの着地点をどこにするかの認識を早めに合わせておくのも大事かもと感じました。
4パターンぐらいあり得ると思っています。(ついでに自分の認識も書いてみました)
- とりあえず今のVOICEVOXのStoreのコードがバグなく動く
- カスタムクラスや関数がtargetやpatchesに来ることを想定しなくて良い(前例がないはず)
- 関数が来ないのでstructedCloneが使える
- カスタムクラスが来ないのでassertもなくてOK
- patches内で関数を使用可能にする
- structedCloneが使えなくなるが、専用のclone関数を実装すればOK
- そもそもVuex.stateに関数を代入する設計が危ない気がするので、関数を想定しなくてもいいかもしれない
- 追加のテストコードが必要
- どんな関数でもサポートするのであればそこそこのテストが必要
- ラムダ関数、グローバル関数、クラスメソッド
- カスタムクラスが来た時にエラーを出す
- ありがたくはあるが、もし可能なら実行時ではなく型エラーが出るとより嬉しい
- 追加のテストコードが必要
- これは1種類ぐらいテストがあれば良さそうなので結構簡単そう
- カスタムクラスを受け入れ可能にする
- 追加のテストコードが必要
- もし全てをサポートするならものすごい大量のテストが必要になる
- メソッド・コンストラクター・プライベートメソッド・プライベート変数、などなど
- VOICEVOXのいろんなところで使われてるstructedCloneがカスタムクラスに対して使えないので、structedCloneしてるとこは全部immerPatchのcloneに移行しないといけない
- となると実行速度がstructedCloneと同程度かのテストも必要
- 追加のテストコードが必要
今のプルリクエストは1に加えて2と4もテスト以外実装されてるという認識です。
関数やカスタムクラスを使うのは例外的な何かがありそうなので、store.stateやmutationではSet・Record・Map・Array以外のObjectの使用を割けていました。
これは僕がJavaScriptやVuexに精通していないが故に、漠然とした不安を感じているだけなのかもしれません。
ただ、もしこの不安がみんなの中にもあるのであれば、一旦検討と議論を挟んでから実装したい気もしました。
提案として、何を実装するかいったんさておき、このような流れにするのはいかがでしょうか。
- 既存の
applyPatches_
のテストを書き、mainブランチに実装する- 自前実装の
applyPatches
が以前と変わっていないことをテストするには、まず以前のコードでテストが通ることを確認してから、テスト変えずにapplyPatches_
を自前実装に変えるのが最も正確なはず - 今のプルリクエストの
immerPatch.spec.ts
をmainブランチにプルリクエストに出していただければ完了? - レビューも分けられるので意外と効果的
- でもお手数おかけしちゃう感じになるので、面倒であれば一緒でもOK
- 自前実装の
applyPatches_
を、関数とカスタムクラスに非対応な状態で自前実装に置き換える- もし将来的に関数やカスタムクラスの実装に致命的なバグがあった場合、最悪revertになるので、すぐに戻せるようにしておきたい
- ちょっとこれだけは他の実装とプルリク分ける形でお願いしたい
- もしくは関数やカスタムクラスが問題ないことを議論し尽くしてから実装
- 関数やカスタムクラスをコマンド・cloneに対応させるかを検討
- VuexやJavaScript、あと設計的に問題ないかを検討しまくる感じになる
- 結構考えるのは楽しいけど、割と時間かかる印象。多分のんびり進めて1.5ヶ月。
- 関数やカスタムクラスを実装
どうして行くかは、わりと僕たちがどうして行きたいかに依存していると思ってます。
個人的な思いとしては、関数をstateに入れられるようにすると、ありとあらゆることができるようになってしまって設計がぶっ壊れるので、だいぶ避けたい気がします。
一方のclone可能なカスタムクラスは、以前チャレンジしようとして難しいことがわかったStrictMap
の悲願だと思います。必要な検討事項がなかなかの量なのでなかなか道は遠そうですが。
あとは当初の目的だったtypescriptのstrict化とどっちを目標地点にするかなのかなと思いました!!
内容
現状はcommandの処理において、immer内部のapplyPatchesを利用している
しかしこれは本来非公開であるAPIを強引に読みこんでいるため将来にわたって互換性が維持される保証が無い
このリスクを軽減するため、applyPatchesを自前で実装してcommandの処理においてそちらを利用するようにする
関連 Issue
#362 のrevertを含みます
スクリーンショット・動画など
その他