search

VOICEVOX / voicevox

無料で使える中品質なテキスト読み上げソフトウェア、VOICEVOXのエディター
https://voicevox.hiroshiba.jp/
Other
2.48k stars 300 forks source link

vuexをやめてpiniaにする #1004

Open sousuke0422 opened 1 year ago

sousuke0422 commented 1 year ago

内容

vuexをやめてpiniaに移行します。 vuexはメンテナンスモードへなったそうです。

Pros 良くなる点

typescriptとの相性が良くなり開発体験が良くなる。

Cons 悪くなる点

そこそこ大きな改修が必要。

実現方法

VOICEVOXのバージョン

0.?.0

OSの種類/ディストリ/バージョン

その他

Hiroshiba commented 1 year ago

幻？のVuex 5があって、それとほぼ同じなのがPiniaなんですね。 移行は実践してくださる方がいれば前向きに検討したいと考えています。

ざっと眺めた感じ大変になりそうだったのは次の２点です。

Fluxではなくなるらしく、mutationが消えると同時にdispatchも消える（ただの関数になる）のですが、まあこの影響はあまりない気がします。むしろ楽になりそう。

ちなみに、複数のstoreがお互いのstateを見るのってどうやって実装すればいいかってご存知だったりしますか･･･？ @sousuke0422 ざっと見た感じ、ここにできると書かれてるけどわからなかったので･･･。

Hiroshiba commented 1 year ago

どちらかというと前向きに検討したいと思ってる理由をメリデメとして列挙してみます。

TypeScriptがちゃんとサポートされてること、順番を気にせず書けることが嬉しいポイントです。 ただまあVOICEVOXは自作ラッパーでそのあたり解決してるので、移行はマストではないなという気持ちです。 これからの新規参入にとってはPiniaのが直感的そう。

Hiroshiba commented 1 year ago

もしよかったら詳しい方からコメントお願いできるととても参考になるので、ぜひ･･･！

@Segu-g さん Command周りどうなりそうかちょっと聞いてみたいです。mutationが消えるので･･･。

@yamachu さん ProxyStore周りがちょっと気になってるのですが、問題になりそうでしょうか。

@MT224244 さん 型周りやstore分割など、全体的な所感を聞いてみたいです。

sousuke0422 commented 1 year ago

ちなみに、複数のstoreがお互いのstateを見るのってどうやって実装すればいいかってご存知だったりしますか･･･？ @sousuke0422 ざっと見た感じ、ここにできると書かれてるけどわからなかったので･･･。

ルールが一応見つかりました・・・ 確かに、情報がまだ少ないのもデメリットですね

Hiroshiba commented 1 year ago

おーーー！！Cookbookの方にあったんですね、見落としてました。 store内で他storeのstateを使う場合はfunction内のみという制約、なるほどぉ。Ref使うとかの手もありそうだし、この辺のデファクトは変わってきそう感。

Segu-g commented 1 year ago

piniaでは同期的にデータの整合性を保つためのmutationに相当する方法として$patchがあるようです。 もし、現行のコマンドシステムをそのままpiniaに移行するならば、この$patchに渡す関数をimmerに通して履歴に入れるcreateCommandMutation相当のラッパーを作れば良さそうです

懸念としては、

逆にストアを分割すればMUTEXを細かく設定して、コマンドを依存ごとに並列に実行するなどもできるかもしれません。

Hiroshiba commented 1 year ago

seguさんありがとうございます！！ $patchをラップする感じ、なるほどです！！

$patchはstore単位でしか更新できないので、storeの分割はそれぞれが整合性として独立する要素が分割上限になる

こちらの懸念については、微妙な策かもですがundo/redoしたい全stateだけを持つstoreを作るとかで解決できるかもと思いました！

Hiroshiba commented 1 year ago

piniaについてちょっと調べてみたのですが、メリットに上げているmutation/action/getterの順番を気にせず書けるは、公式であまり（というかかなり）推されてない感がありました。 もし導入を検討するにしても、時間をおいてかなり様子を見たほうが良いのかなという気持ちでいます。

Segu-g commented 1 year ago

前のコメントで

patchを適用と同時にcommandsを変更するためにはcommandsと変更するストアが同一のストアである必要がある

と書きましたがpiniaは同期的にstateを更新できるようなので、非同期的なコードを挟まない限り整合性は保証されそうでした。

Hiroshiba commented 1 year ago

piniaへの移行、現実的なのかどうか若干見積もれないですね…。 コマンドはstate統一か何かしらの制約でなんとかなるとして、規模が規模なので…。

ちょっとずつ移行する方法も思いつかない。うーん。

Hiroshiba commented 1 year ago

undo/redoも含め、piniaを用いたときの設計を考えてました！！

vuexからの移行を考えると、fluxじゃなくなる点も考慮ポイントっぽかったです。 といってもviewからstateを直接変えられないようにすれば十分なので、stateにreadonlyを付けてreturnすれば良さそう？

コマンドに関してはstoreを跨ぐときのことを考える必要がありそうです。 全ストアを1箇所に集めた最強ストアを作るのと、小分けのストアに対して頑張って$patchを適用するの、どちらで行くかって感じかなと！

↓なんとなくこんな感じかなというコードを書いてみました。（文法正しくないので動かないです）

const commandStore = (otherStores: Store[]) => {
    const redoCommands = Command[];
    const undoCommands = Command[];

    const createCommand = (stores: Store[], mutation: Mutation) => {
        // どういう実装？

        // immerのcreateDraftを作る場合（できる？）
        return () => {
            const drafts = stores.map((store) => immer.createDraft(store.state));
            mutation(...drafts);
            const redoPatches1, undoPatchs1 = immer.finishDraft(drafts[0]);
            const redoPatches2, undoPatchs2 = immer.finishDraft(drafts[1]);

            redoCommands.push(Command(redoPatches1 + redoPatches2)); // まとめれる？
            undoCommands.push(Command(undoPatchs1 + undoPatchs2));

            store1.$patch(() => {apply(redoPatches1)})
            store2.$patch(() => {apply(redoPatches2)})
        };

        // パッチを分解して各Storeに渡す場合
        return () => {
            const redoPatches, undoPatchs = immer.produceWithPatches(mutation);

            redoCommands.push(Command(redoPatches));
            undoCommands.push(Command(undoPatchs));

            const redoPatches1 = filter(redoPatches)
            store1.$patch(() => {apply(redoPatches1)})

            const redoPatches2 = filter(redoPatches)
            store2.$patch(() => {apply(redoPatches2)})
        };
    }

    return {
        createCommand,
    }
}

const store1 = (storeName: string) => {
    const commandStore = useCommandStore();

    const getters = {}; // ちゃんと型つく？
    const actions = {}; // ちゃんと型つく？

    const state = reactive({
        count: 0,
    })

    const mutationA = (state) => {state.count++};

    actions.commandActionS = commandStore.createCommand(this, (draft) => { draft.count++ });
    actions.commandActionT = commandStore.createCommand(this, (draft) => { mutationA(draft) });

    // コードジャンプはちゃんとここになる？
    actions.actionX = () => {this.$patch(() => state.count++ );};
    actions.actionY = () => {this.$patch((state) => mutationA(state));};

    getters.getterX = computed(() => state.count * 10); // WritableComputedは禁止

    return {
        ...readonly(state), // ちゃんとreactiveに解体できる？
        ...getters,
        ...actions,
    }
}

const store2 = (storeName2: string) => {
    const commandStore = useCommandStore();
    const store1 = useStore1();

    const state = reactive({
        count2: 0,
    })

    actions.commandActionU = createCommand(
        [this, store1],
        (draft, draft1) => {draft.count2++; draft1.count++;},
    );

    return {
        ...readonly(state),
        ...actions,
    }
}

メモ

Segu-g commented 1 year ago

@Hiroshiba 上のコードだとcreateCommandはstore自身を引数として与える必要がありますが、compositionAPIではstore自身を参照することができないようです

解決策としては

composition styleはmutation/action/getterの順番を気にせず書けるといった目的があると思いますが、やはり面倒が多そうといったイメージになってしまいますね・・・

Hiroshiba commented 1 year ago

@Segu-g あ～～～　なるほどです！！

storeを別のstoreでラップするを拡張して、commandの操作対象なstateだけcommandStoreに乗っけるという手もありそうだなと思いました。 これだとstateが複数storeに分断されることを考えなくてすみますし、どれがcommandの対象なのかがわかりやすいですし。 （初期化用途など、command操作なしでstateを変える手を用意しないとですが）

他の手としては、将来$patchが実装されることを見越して$patchをsetup funcitonで定義するも全然ありに思います。 書式合ってない気がしますが、こんな感じでしょうか。

function patchFactory<S>(s: S) {
  return (func: (s: S) => void) => {
    func(s)
  }
}

const $patch = patchFactory(state)

次手としてstoreを別のstoreでラップするもありに思います。stateだけ持つstoreを毎store作る感じですよね。 object形式で書くはまたVuexの苦しい感じに戻ってしまうので避けたいですね･･･。 （commandをactionではなく只のfunctionとして定義する。はちょっとイメージできなかったです）

Segu-g commented 1 year ago

$patchをsetup funcitonで定義する は型的に面倒な気がしたので storeを別のstoreでラップする の形式で小さめのサンプルを書いてみました。

https://github.com/Segu-g/pinia-composition-command-mre/blob/master/src/stores/textStore.ts

import { defineStore } from 'pinia';
import { ref } from 'vue';

import { defineCommand } from './command';

export const useText = defineStore('text', () => {
  const text = ref('');
  return {
    text,
  };
});

export const useTextCommand = defineStore('textCommand', () => {
  const textStore = useText();
  const changeText = defineCommand({ textStore }, ({ textStore }, text) => {
    textStore.text += text;
  });
  return { changeText };
});
Hiroshiba commented 1 year ago

@Segu-g おー！！！！！ コードも読みました、かなりスッキリしている印象です！！！ いくつかこうできたら最高だなという設計上の希望が思いついたのでちょっと列挙しています 🙇

  1. defineCommand内でuseCommandを実行しちゃってる点
    • Vueのコンポーザブルがまだしっかり固まっていないからstatic function内で呼んでも問題ありませんが、いつか問題になるかもしれないなと思いました！
    • defineCommandの第1引数にcommandStoreを渡す設計でもいいかも
  2. Storeがコマンドに対応しているかどうかわからない点
    • useCommand内でuseStoreしているものがコマンドに対応していますが、それが型からは分からないかも？
  3. pushCommandが露出してしまっている点
    • まあこれは仕方なさそう
    • $pushCommandなどとして特殊なものだということがわかるようにするといいかも
  4. stateをstoreの外から直接変更不可にできない点
    • これが一番むずかしい課題かも

１～３はなんとかなりそうなのですが、４だけは解決策を思いつかないでいます。。 全storeを1ファイルにまとめて、state定義用のstoreにはexportをつけないようにし、全stateをuseしたあとreadonlyを付けてreturnすれば行けるかもですが･･･。

Segu-g commented 1 year ago

@Hiroshiba

defineCommand内でuseCommandを実行しちゃってる

defineCommand自体がuse系関数の感覚で書いてましたが確かに命名的にも非自明でしたね。毎回

const commandStore = useCommand();
const commandA = defineCommand(commandStore, ()=>{})

のようにするのも冗長かなと感じたので、

const { defineCommand } = useCommandContext();

と書けるくらいのヘルパー関数は書いてもいいと思うのですがどうでしょうか？

Storeがコマンドに対応しているかどうかわからない点

undo, redoの時はcommandから各ストアの$patchを呼ぶ必要があるため逆参照用の登録が必要になっちゃうんですよね・・・とりあえず以下のように登録する配列を定義して、その中のストアしか渡せないように型パズルしました。

const getUseStoreArr = () => [useCounter, useText];

stateをstoreの外から直接変更不可にできない点

プロパティを（型的に）Readonlyにすること自体はreturn { ...toRefs(readonly(state)) };だったり以下みたいな型書き換えを使えばできます。

// storeHelper.ts
import { StoreDefinition } from 'pinia';
import type { DeepReadonly } from 'ts-essentials';

type ToReadonlyStoreDefinition<SD> = SD extends StoreDefinition<
  infer Id,
  infer S,
  infer G,
  infer A
>
  ? StoreDefinition<Id, DeepReadonly<S>, G, A>
  : SD;

export function toReadonlyStoreDefinition<SD>(useStore: SD) {
  return useStore as ToReadonlyStoreDefinition<SD>;
}

// index.ts

import { useCounter as _useCounter } from './countStore';
import { useText as _useText } from './textStore';
import { toReadonlyStoreDefinition } from './storeHelper';

export const useCounter = toReadonlyStoreDefinition(_useCounter);
export const useText = toReadonlyStoreDefinition(_useText);

今気づいたのですが今のコードはmutationの型がDraft<State> => voidなのでDraftの効力でreadonlyが無効化されてますね... readonlyなプロパティをdefineCommandで変更してしまう可能性がありそうです。

PS

Draftを外したところちゃんとReadonlyなStoreはmutation内でも操作できないことを確認しました。 なのでReadonlyを付けるならCommandStore(wrapしてるStore)でstateを再exportするか、index.tsなど別の場所で型を変えるなどの方法があると思います。

Hiroshiba commented 1 year ago

useCommandContext(); と書けるくらいのヘルパー関数は書いてもいいと思うのですがどうでしょうか？

なるほどコンポーザブルなんですね！ 良さそうなのかなと思いました！！

stateをstoreの外から直接変更不可にできない点

プロパティを（型的に）Readonlyにすること自体はreturn { ...toRefs(readonly(state)) };だったり以下みたいな型書き換えを使えばできます。

stateの方のstoreだけ型を置き換えたりラップしたりするのなるほどです！！ 利用側は２つインポートする必要がありますが、良さそうに思いました！

（ちょっと別の方法としては、今stateを持っている方のStoreを_useTextStateStoreとかにして、コマンドがある側をuseTextなどとし、useText側でstateをreadonlyにしてreturnするのもありかなとかちょっと思いました。 ただこの方法だとstate１つ１つををreturnに書いていく必要があって定義側がめんどくさいですね！！）

Hiroshiba commented 1 year ago

なんかpiniaに置き換えること自体は通れる道な気がしました！！ その付随効果として何があるかをちょっと考えてみました。

  1. action相当の処理内でmutationを実行していること
    • これはpiniaがそもそも内部でmutexを持っていないので、全部$patchで書いたところでaction相当になる
    • そもそもこれがダメっぽいのであればpiniaに依存できなさそう 😇
    • 若干危険な香りはしていますが、まあ大丈夫なんじゃないかなと思ってはいます。。
  2. interfaceがなくなること
    • 今まではstore/type.tsにインターフェースと実装が１つずつある形でした
    • これが実装オンリーになるので、ちょっと有識者たちの意見をもらった方が良さそうに思います
    • インターフェースが別れてある目的としてはやっぱりモックを作りやすいことだと思うので、仮にpiniaに変えた時にモックに差し替えられるのかの目処が立っていると良さそう？
    • pinia testing見た感じspy作成はできそう？
  3. そもそもpiniaがいらないかもしれないこと
    • piniaで使ってる機能は$patchぐらい？
    • 制約がなさすぎるのも難しいかもなので、とりあえずpinia使うのはありかも

1と2に問題があるかどうかをいろんな人に聞いてみるフェーズかなと思いました！！

Segu-g commented 1 year ago

ちゃんと検証は済んでないですが1の問題は多分大丈夫だと思います。

javascriptだとその言語的な特性から(piniaがWorkerとか建ててなければですが)実行されるスレッドは常に1つで、 それらはawaitなどのコールーチンの境界にならないと手放されない筈です。 なのでdefineCommand内のコードは同期的に実行され、他のコマンドと変更順序が変化する余地はおそらく無いです！

そもそもvuexでcreateCommandAcitonが上手くいかない原因がcommitしたpatchが適応されるタイミングが非同期であることだったので、$patchで同期的に状態が書き換えられるpiniaでは問題にならないでしょう。

Hiroshiba commented 1 year ago

なるほどです！！！　ということはまあ以前と一緒の使い勝手になる感じですかね･･･！！ （action内のcommit/state変更のタイミングによって前後する可能性があるというだけ）

Segu-g commented 1 year ago

（ちょっと別の方法としては、今stateを持っている方のStoreを_useTextStateStoreとかにして、コマンドがある側をuseTextなどとし、useText側でstateをreadonlyにしてreturnするのもありかなとかちょっと思いました。 ただこの方法だとstate１つ１つををreturnに書いていく必要があって定義側がめんどくさいですね！！）

そもそもスプレッドするために付けなくちゃいけない storeToRefs を付けたら$idとかの後天的なプロパティも消えてくれたのでこの方式で書きたいと思います。

return { commandChangeText, ...storeToRefs(textStore) };
Hiroshiba commented 1 year ago

storeToRefs良いですね！！

Hiroshiba commented 1 year ago

ルールを整備したりしないと結構危うい気がしてきたのでちょっと考えてみました！

で、実装としての候補はこんな感じ･･･？

  1. 実装が簡単な方に倒す
    • stateをreadonlyにしない
    • actionはmutation(state)を直に叩く
    • action内でstateを書き換えられるのは人の目で抑止
    • mutation内でactionが呼べるのは人の目で抑止
  2. $patchのみでstate変更できるようにする
    • stateをreadonlyに
    • actionはpatch経由でmutation実行
    • mutationを簡単にpatch実行できるラッパーが必要？
    • mutation内でactionが呼べるのは人の目で抑止

個人的にはとりあえず1でいいのかなとかちょっと思ってます。 ちょっと人を信用しすぎてるかもしれないですが。。 😇 （あとESLintで抑止できるんじゃないかな～～と思ってます）

Segu-g commented 1 year ago

上記のコメントを踏まえて少しサンプルを書いてみました。 https://github.com/Segu-g/voicevox/blob/feature/pinia-mutation-patch-style/src/pinia-stores/preset.ts

方針としてはstateを持つstoreは$patch利用のためにusePresetStateStoreのように分割し、presetStoreでuseStoreAsStateヘルパー関数を介して用います。 useStoreAsStateはstateを定義したstoreからMutationやreadonlyなstoreを取り出すためのヘルパーで、先ほどの方針でいけば (2. $patchのみでstate変更できるようにする) の方を実装したものになります。 Mutationの定義はdefMutで. Actionの定義はdefActを通して定義することが可能であり以下のような形になります。


export const usePresetStore = defineStore("preset", () => {
  const { state, defMut, defAct } = useStoreAsState(usePresetState);

  // getter
  const defaultPresetKeySets = computed(() => {
    return new Set(Object.values(state.defaultPresetKeys));
  });

  // action
  const setDefaultPresetMap = defAct(
    async ({
      defaultPresetKeys,
    }: {
      defaultPresetKeys: Record<VoiceId, PresetKey>;
    }) => {
      window.electron.setSetting("defaultPresetKeys", defaultPresetKeys);
      setDefaultPresetMapMut.act({ defaultPresetKeys });
    }
  );

  // mutation
  const setDefaultPresetMapMut = defMut(
    (
      state,
      { defaultPresetKeys }: { defaultPresetKeys: Record<VoiceId, PresetKey> }
    ) => {
      state.defaultPresetKeys = defaultPresetKeys;
    }
  );

  return { ...storeToRefs(state) , defaultPresetKeySets, setDefaultPresetMap };
}):
Segu-g commented 1 year ago

また別の話になりますがMutationの定義としてstoreは引数から与えられたもののみを対象として操作を記録する必要がありますが、Mutationから他のstoreをgetする時にも引数からstoreを参照する必要があって面倒なことに気づきました。

これはMutaitonの実行中に参照しているstateが書き換えられない場合は問題ないのですが, commandの場合はstateの変更の適応タイミングが遅いので、過去の値を参照してしまう可能性が出てきてしまいます。

Commandの引数に取るMutationは複数のStateを取れる型定義になっているので問題ないのですが、外のstoreを引数を介さずに参照してしまう危険性は高そうだなと思いました。

const changeText = defineCommand({ textStore, hogeStore }, ({ textStore, hogeStore }, text) => {
    textStore.text += text;
    hogeStore.count += 1;
  });
Hiroshiba commented 1 year ago

ちょっといろいろ考えたのですが全然まとまってないけど一旦ちょっとコメントだけ･･･！

アイデア1

今のVuexラッパーみたいにして書いて、pinia用のobjectにする

  functionName: {
    mutation: (state) => {},
    action: ( {state, actions, mutations} ) => {}
  }

アイデア2

mutをstateStore側に書く。 （他のstateが見れないという課題がある）

アイデア3

module直下にmutationを書き、他stateも渡すようにする （@Segu-g さんの案）

アイデア4

prefixかsuffixにactやmutを必須に。

Hiroshiba commented 1 year ago

別のアイデアが思い浮かんだのでメモ。

アイデア5

piniaのdefine関数内でmutationやactionを定義するとき、専用の型を使うようにする。 ESLintでmutationが正しそうなことを確認する。 （例：stateという言葉を使用禁止、外部変数のキャプチャ禁止など）

ESLintで使用禁止や外部変数の利用禁止するのはChatGPTくんに聞いた感じできるかも･･･？ https://chat.openai.com/share/cce984ef-c391-47a5-ab4e-e79d17b60ca7

Hiroshiba commented 11 months ago

@Segu-g pinia化ですが、記憶がまだ残っているうちにundo/redo部分まで進めたい気持ちがあります･･･！ アイデア5の方針で行きつつ、ESLintでmutationに制限をつけるのは後回しにするという流れで行くと一旦試作ができるかもと思ったのですが、どうでしょう･･･？ 👀

Hiroshiba commented 11 months ago

@Segu-g pinia化、まだ記憶が残っているうちに進めておかないと多分もったいないので、ちょっと危機感を感じ始めました･･･！ せっかくだから引き継いでみようかなと思ったのですが、コマンド付きmutationがやっぱり鬼門な感じでしょうか？ であればより制約を強めて、storeがそれぞれ分かれているタイプを諦めて全ストアを統一し、あとは各単元ごとにmutationなりactionなりを実装する形とか目指すとかもありかも？

Hiroshiba commented 11 months ago

ESLintの設定を頑張ろうとしてみてたのですが、動かす方法がよくわかりませんでした･･･。 とりあえず、ローカルのESLintルールを作るのはeslint-plugin-local-rulesを使うのが良さそう、というところまで進みました 😇 https://github.com/Hiroshiba/voicevox/commit/f9a2dde47e5c0676be157c3e9a35f0b486bcd9e1

Segu-g commented 10 months ago

@Hiroshiba 何度か書いていたのですが、今まで出た全ての要望を全て満たすようなラッパーを書こうとすると、設計力不足で過剰に複雑なインターフェースになってしまいますね...

とりあえず書いてみたものがこちらになります

stateを定義するときはdefineCommandableStateを用いて, その出力を元にuseContextを叩くことでdefMutやdefGet等の補助関数を定義できる形です。

import { defineStore, storeToRefs } from 'pinia';
import { defineCommandableState } from './command';
import { useStore } from '@/vuex-store';

export const CountState = defineCommandableState({
  id: 'count/state',
  state: () => ({
    counter: 0,
  }),
});

export const useCount = defineStore('count', () => {
  const { state, defMut, asCmd } = CountState.useContext();
  const increment = defMut((state) => {
    state.counter += 1;
  });
  const countUpWithVuex = () => {
    const vuexStore = useStore();
    vuexStore.commit('increment');
  };
  return {
    state: storeToRefs(state),
    commandIncrement: asCmd(increment),
    countUpWithVuex,
  };
});

このブランチではdefGetやdefMutはあくまで型を補完するための補助関数でしかなく, defMutの戻り値は引数の関数(state, ...payload) => voidがそのまま返されるようにしました。 これらの関数をGetterやActionとして扱うには同じくuseContext()から与えられるgetRef(getter)やasAct(mutation)を経由する必要があります。 提示したサンプル例ではdefMutで定義したincrementをasCmdでCommand化してexportしています.

Segu-g commented 10 months ago

インターフェースを改良しました. asActやgetRefの代わりにgetter.getやmutation.commit()で直接RefやActionとして呼べるようにしました https://github.com/Segu-g/pinia-composition-command-mre/tree/feature/defGetdefMut/ver1

export const TextState = defineCommandableState({
  id: 'text/state',
  state: () => ({
    text: '',
    name: '',
  }),
});

export const useText = defineStore('text', () => {
  const { state, defGet, defMut, asCmd } = TextState.useContext();

  // mutation
  const changeTextMut = defMut((state, text: string) => {
    state.text = text;
  });
  const changeNameMut = defMut((state, text: string) => {
    state.name = text;
  });
  // action
  const changeTextAndName = (text: string) => {
    changeNameMut.commit(text);
    changeNameMut.commit(text);
  };
  // command
  const commandChangeText = asCmd(changeTextMut);
  // getter
  const textGet = defGet((state) => state.text);
  const isTextSameToName = defGet((state) => state.name == textGet(state));

  return {
    state: storeToRefs(state),
    changeTextMut,
    changeNameMut,
    changeTextAndName,
    commandChangeText,
    textGet,
    isTextSameToName,
  };
});
Hiroshiba commented 10 months ago

ありがとうございます！！！！！！すごく勉強になりました！！！

defCmdとasCmdがありますが、defCmdは結構リッチなのとasCmdでの書き換えが割と想像しやすいので、シンプルにasCmdの方だけでもいいかもと思いました！

あとdefGetは他のstoreのstateやgetterを使えることがわかるように、stateを与えなくても良いようにできるかも？ getCharacterInfoでgetterからgetterを呼ぶ需要はあるので、そこは残すと嬉しそう！

mutationの中でaction（呼んではいけない関数）を実行しているのかどうかが分かりにくいかもと思いました。 以下ちょっと考えてみ案です。

案

Hiroshiba commented 10 months ago

pinia化の流れですが、audio.tsの置き換えはコマンドを一気に置き換えないといけない、つまり3000行にわたる全てのコードの置き換えを一気にやる必要がある気がしています。 これを実現するのはかなり大変で、多分1回audio.tsを通行止めにして1週間ほど停止させ、その期間に一気に置き換えることを目標にするのが良い気がしました！

この点で一番困りそうなのが、API構成周りが現状のコードと合いそうかどうかという点です。 別リポジトリに書いている時には気づかなかった問題があるかもと感じています。

そこで提案なのですが、1回実際に一部のコマンドを実装してみて、それをプルリクエストにしていただいてレビューし、そこが固まってからaudio.ts全体を変更していく方針はどうでしょうか？ 多段階になってしまうのでお手間いただく感じになってしまうのですが、一気に置き換えるよりは心理的にもやりやすいのかなと思った次第です。（お互いに･･･！！）

コードを読んでいましたが、コマンドの中だとAudioItemを足すCOMMAND_REGISTER_AUDIO_ITEMと、消すCOMMAND_MULTI_REMOVE_AUDIO_ITEM辺りが簡単なように見えました。 https://github.com/VOICEVOX/voicevox/blob/37d52de4647b7e19bbac7d42cde06deb295c5050/src/store/audio.ts#L1850 この2つだけとりあえず実装してみてプルリクを出していただく･･･という進め方はいかがでしょうか･･･！！ @Segu-g

Segu-g commented 10 months ago

1666 にてデモ実装をしてみました。

実装の元となったリポジトリは以下になります. https://github.com/Segu-g/pinia-composition-command-mre/blob/feature/defGetdefMut/ver2/src/stores/textStore.ts

defCmdはaction内で呼んでいるcommitをmutationとcommandで切り替える需要が大きいため残しています。

前との差分としては

です

Hiroshiba commented 10 months ago

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

typescriptのreadonlyは安全ではないのでBrandを用いてreadonlyなstateをwritableなstateに代入できないように

素晴らしいと思います！！！！

Hiroshiba commented 8 months ago

ESLintで独自の型チェックを細かくする方法が分からなかったのですが、こちらに頂いたプルリクエストがとても参考になると思うのでメモです！！