applyPatchesを自前実装に置き換える

[White-Green]

内容

現状はcommandの処理において、immer内部のapplyPatchesを利用している
しかしこれは本来非公開であるAPIを強引に読みこんでいるため将来にわたって互換性が維持される保証が無い
このリスクを軽減するため、applyPatchesを自前で実装してcommandの処理においてそちらを利用するようにする

関連 Issue

#362 のrevertを含みます

スクリーンショット・動画など

その他

[Hiroshiba]

ちょっとお力お借りしたく思いアサインさせていただきます 🙇
@sevenc-nanashi @Segu-g

[Segu-g]

@White-Green
ここの負債を書いてしまったのは私です🙇‍♂️
修正PRを出してくださってありがとうございます🙇‍♂️

実装見ました！
2つだけ気になったことがあります。

1つ目はこのコードはImmerのapplyPathesを自前実装したものですが、厳密にimmerのapplyPathesの仕様を満たしてはいない点です。例えばImmerはユーザーが独自定義したクラスが扱えますがこのコードでは扱えません。
それ自体は問題ないと思うのですが、immerのapplyPatchesと同じように使おうとして事故が起こり得るので、どんなオブジェクトに対応しているかコメントを書いた方が良いと思います！

2つ目はカスタムクラスが入ってきた時にエラーが出ないことです。
structuredCloneはユーザーが定義したクラスに対しても正常系で処理できますが、prototypeを継承しないプレーンオブジェクトになってしまうという問題があります。
特にCommand周りはコード的に隠ぺいされているので、この動作はStoreに間違ってカスタムクラスを登録した時にバグの原因を特定するのが困難になりそうです。builtinのクラス以外の場合はエラーを出すなどは可能でしょうか？

以上、確認をよろしくお願いします！

[White-Green]

@Segu-g コメントありがとうございます

immerのapplyPatchesと同じように使おうとして事故が起こり得るので、どんなオブジェクトに対応しているかコメントを書いた方が良いと思います！

現状問題が想定できるのは独自classぐらいだと思うので、独自class非対応の旨のみコメントを入れておきました

builtinのクラス以外の場合はエラーを出すなどは可能でしょうか？

こちらはどうしようか悩んでいるところです
例えばMyClassをcloneしたときに検知するにはclone後にvalue?.__proto__ !== cloned_value?.__proto__などで可能ですが、この方法では{ field: MyClass }やMap<number, MyClass>のようなものには対応できません
これを再帰的に確認していくようにすることはできますが、開発時のフールプルーフ機構としては必要以上に複雑なものになるのではないかと思っています
つまり、現状はこの実装に関して以下の選択肢があり、どれをとろうか決めかねている状態です

• この検査機構を実装しない
  • pros: 最もシンプル
  • cons: 独自classを間違って利用した場合に発見の難しいバグになる可能性がある
• 1段のみ検査する(MyClassを直接cloneした場合のみ検知する)
  • pros: 実装が複雑になりすぎない(フールプルーフ機構としてはちょうどいいボリュームだと思います)
  • cons: 場合によっては検査をすり抜ける
• 再帰的に全て検査する
  • pros: フールプルーフ機構としての目的をほぼ100%達成できる
  • cons: 実装が複雑になる(このぐらい複雑になるのであればstructuredCloneの採用を止めた場合と同等の規模の実装になるが、structuredCloneを採用したのは自前実装した場合よりもシンプルに保ちながらも信頼できるからであり、その利点がほぼ消える)

このようなトレードオフに関してVOICEVOXでの方針などがありましたら教えていただけますでしょうか

[Segu-g]

@White-Green

今気づいたのですが、このコードはMap<string, Function>等の「structedCloneが不可能だが処理したい」オブジェクトについての処理について記述が抜けていますね。
現状のコードはstructuredCloneで対応しきれないfunctionの対応についてここで対処しているため、上記のような型に対応するならこの部分も変更する必要がありそうです。

これを再帰的に確認していくようにすることはできますが、開発時のフールプルーフ機構としては必要以上に複雑なものになるのではないかと思っています

structuredCloneを利用する前提での議論ですね。前述のstructuredClone不可なオブジェクトに対する処理を含めるなら、再帰的な走査に相当する処理は必要になってしまうのかなと思います。

若しくは、このプロパティの利用箇所であるcommandが対象とするStoreに対して静的検査を用いて型に制約を掛けてしまうのもアリだと思います。structuredCloneの対応型を明示して列挙する必要がありますが、コードが対応している型を表現できるならフールプルーフ機構として動作するかも？型を工夫すればMap<string, Function>も弾けますし。

playground例

[White-Green]

@Segu-g

Map<string, Function>等

これは純粋に見落としていました
気づいていただきありがとうございます

独自classなどをcloneした際に検知するフールプルーフの機構ですが、とりあえず再帰的にすべてを確認する方針のものを実装してみました
複雑すぎるようであれば機能を一部落とすことも視野にレビューいただければと思います

[Segu-g]

フールプルーフの実装ありがとうございます！

確かに実装の複雑さ（コードの量）が上がりますね....

提案なのですが、現在の「structuredCloneを試してからダメだった場合コンテナのアイテムを走査して再帰する」形から「コンテナのアイテムを走査して再帰し、コンテナではない葉に当たった場合にstructureCloneをする」形に変更するのはどうでしょうか？
というのも、現在のコードではネストの深くにfunctionが存在した場合、ネストの数だけstructureCloneを失敗してしまうことになり効率が悪そうだと感じます。
サポートすべきコンテナはplain Object, Array, Map, Setぐらいだと思うので、それを辿ってコピーすることでstructuredCloneを行う場所を葉のみに限定することができ、assertもそこに入れこめるのかなと思います。

// 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.
このパターンだと非整礎的なオブジェクト(cyclic なオブジェクト)のcloneができませんね。
そんなオブジェクトを処理するべきかは謎ですが....

type T = { x: T }
const x: T = {} as T
x.x = x
console.log(structuredClone(x))

[White-Green]

フールプルーフ機構も含めてコードが複雑になってしまったので、むしろstructuredCloneを使わない方向で整理しました
ついでに独自classも使えるようになりました

[Hiroshiba]

@Segu-g さんありがとうございます！！

@White-Green
時間ができたので見させていただきました！
とても勉強になりました！！

現在は設計がある程度のパターン出てきたフェーズだと感じています。
あとちょっともしかしたら裾が当初の目的よりも広がっているのかもと感じました。
ということで別視点ですが、着地点をどこにするか相談させてください！

---

個人的に、このプルリクエストの着地点をどこにするかの認識を早めに合わせておくのも大事かもと感じました。
4パターンぐらいあり得ると思っています。（ついでに自分の認識も書いてみました）

1. とりあえず今のVOICEVOXのStoreのコードがバグなく動く
   • カスタムクラスや関数がtargetやpatchesに来ることを想定しなくて良い（前例がないはず）
   • 関数が来ないのでstructedCloneが使える
   • カスタムクラスが来ないのでassertもなくてOK
2. patches内で関数を使用可能にする
   • structedCloneが使えなくなるが、専用のclone関数を実装すればOK
   • そもそもVuex.stateに関数を代入する設計が危ない気がするので、関数を想定しなくてもいいかもしれない
   • 追加のテストコードが必要
     • どんな関数でもサポートするのであればそこそこのテストが必要
     • ラムダ関数、グローバル関数、クラスメソッド
3. カスタムクラスが来た時にエラーを出す
   • ありがたくはあるが、もし可能なら実行時ではなく型エラーが出るとより嬉しい
   • 追加のテストコードが必要
     • これは１種類ぐらいテストがあれば良さそうなので結構簡単そう
4. カスタムクラスを受け入れ可能にする
   • 追加のテストコードが必要
     • もし全てをサポートするならものすごい大量のテストが必要になる
     • メソッド・コンストラクター・プライベートメソッド・プライベート変数、などなど
   • VOICEVOXのいろんなところで使われてるstructedCloneがカスタムクラスに対して使えないので、structedCloneしてるとこは全部immerPatchのcloneに移行しないといけない
     • となると実行速度がstructedCloneと同程度かのテストも必要

今のプルリクエストは1に加えて2と4もテスト以外実装されてるという認識です。

関数やカスタムクラスを使うのは例外的な何かがありそうなので、store.stateやmutationではSet・Record・Map・Array以外のObjectの使用を割けていました。
これは僕がJavaScriptやVuexに精通していないが故に、漠然とした不安を感じているだけなのかもしれません。
ただ、もしこの不安がみんなの中にもあるのであれば、一旦検討と議論を挟んでから実装したい気もしました。

提案として、何を実装するかいったんさておき、このような流れにするのはいかがでしょうか。

1. 既存のapplyPatches_のテストを書き、mainブランチに実装する
   • 自前実装のapplyPatchesが以前と変わっていないことをテストするには、まず以前のコードでテストが通ることを確認してから、テスト変えずにapplyPatches_を自前実装に変えるのが最も正確なはず
   • 今のプルリクエストのimmerPatch.spec.tsをmainブランチにプルリクエストに出していただければ完了？
   • レビューも分けられるので意外と効果的
   • でもお手数おかけしちゃう感じになるので、面倒であれば一緒でもOK
2. applyPatches_を、関数とカスタムクラスに非対応な状態で自前実装に置き換える
   • もし将来的に関数やカスタムクラスの実装に致命的なバグがあった場合、最悪revertになるので、すぐに戻せるようにしておきたい
   • ちょっとこれだけは他の実装とプルリク分ける形でお願いしたい
   • もしくは関数やカスタムクラスが問題ないことを議論し尽くしてから実装
3. 関数やカスタムクラスをコマンド・cloneに対応させるかを検討
   • VuexやJavaScript、あと設計的に問題ないかを検討しまくる感じになる
   • 結構考えるのは楽しいけど、割と時間かかる印象。多分のんびり進めて１．５ヶ月。
4. 関数やカスタムクラスを実装

---

どうして行くかは、わりと僕たちがどうして行きたいかに依存していると思ってます。

個人的な思いとしては、関数をstateに入れられるようにすると、ありとあらゆることができるようになってしまって設計がぶっ壊れるので、だいぶ避けたい気がします。
一方のclone可能なカスタムクラスは、以前チャレンジしようとして難しいことがわかったStrictMapの悲願だと思います。必要な検討事項がなかなかの量なのでなかなか道は遠そうですが。
あとは当初の目的だったtypescriptのstrict化とどっちを目標地点にするかなのかなと思いました！！