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

vuexをやめてpiniaにする #1004

Open
3 tasks
sousuke0422 opened this issue Nov 2, 2022 · 37 comments
Open
3 tasks

vuexをやめてpiniaにする #1004

sousuke0422 opened this issue Nov 2, 2022 · 37 comments
Labels
優先度:低 機能向上 要議論 実行する前に議論が必要そうなもの

Comments

@sousuke0422
Copy link
Contributor

sousuke0422 commented Nov 2, 2022

内容

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

Pros 良くなる点

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

Cons 悪くなる点

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

実現方法

VOICEVOXのバージョン

0.?.0

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

  • Windows
  • macOS
  • Linux

その他

@Hiroshiba Hiroshiba added 機能向上 要議論 実行する前に議論が必要そうなもの 優先度:低 labels Nov 2, 2022
@Hiroshiba
Copy link
Member

Hiroshiba commented Nov 2, 2022

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

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

  • mutationがなくなること
    • 移行がまあまあ大変そう
    • 特にUndo/Redoを司るCommand周りの実装はmutationと密結合してるので、そもそも引きはがせるかの検討が必要そう
  • storeを分けたほうが良いこと
    • たぶんマストではないけど、Piniaは超大きなstoreを持つのではなく、適切にstoreを分けることを想定してそう
    • まあこっちは、1回超大きなstore作ったあと、リファクタリングとして分割していくと良さそう

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

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

@Hiroshiba
Copy link
Member

Hiroshiba commented Nov 2, 2022

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

  • メリット
    • TypeScriptサポート
    • dispatchが要らなくなり、文字列で扱う箇所が消えるため補間が素直になる
    • mutation/action/getterの順番を気にせず書ける
    • コードジャンプができる
  • デメリット
    • mutationがなくなるのでマイグレーションが大変
    • ↑の影響でCommand周りの実装見直しが必要
    • ProxyStoreの実装も見直しが必要かも
  • 一長一短
    • storeが分割可能
      • 便利そうな一方、ノウハウが蓄積されておらず、試行錯誤しながらになる
    • Composition APIっぽく書ける
      • こちらもノウハウがないから試行錯誤しそう
    • 型宣言が不要になる
      • 1つ関数を作るために書かないといけないコードは減るけど、見通しもちょっと悪くなる

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

@Hiroshiba
Copy link
Member

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

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

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

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

@sousuke0422
Copy link
Contributor Author

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

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

@Hiroshiba
Copy link
Member

Hiroshiba commented Nov 2, 2022

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

@Segu-g
Copy link
Member

Segu-g commented Nov 9, 2022

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

懸念としては、

  • piniaで$patchをmutationのように使う意義がある
  • $patchはstore単位でしか更新できないので、storeの分割はそれぞれが整合性として独立する要素が分割上限になる
  • patchを適用と同時にcommandsを変更するためにはcommandsと変更するストアが同一のストアである必要がある(この部分は実装を上手くやれば回避できるのでそこまで重要じゃないかも)
    くらいです。

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

@Hiroshiba
Copy link
Member

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

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

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

@Hiroshiba
Copy link
Member

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

@Segu-g
Copy link
Member

Segu-g commented Nov 15, 2022

前のコメントで

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

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

@Hiroshiba
Copy link
Member

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

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

@Hiroshiba
Copy link
Member

Hiroshiba commented Jul 21, 2023

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,
    }
}

メモ

  • すべてのStateを1箇所に集約するのでも良いかも

@Segu-g
Copy link
Member

Segu-g commented Jul 22, 2023

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

解決策としては

  • $patchをsetup funcitonで定義する
  • object形式で書く
  • storeを別のstoreでラップする
  • commandactionではなく只のfunctionとして定義する。
    などがあると思います。

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

@Hiroshiba
Copy link
Member

Hiroshiba commented Jul 22, 2023

@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
Copy link
Member

Segu-g commented Jul 23, 2023

$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
Copy link
Member

Hiroshiba commented Jul 24, 2023

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

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

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

@Segu-g
Copy link
Member

Segu-g commented Jul 26, 2023

@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
Copy link
Member

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

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

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

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

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

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

@Hiroshiba
Copy link
Member

Hiroshiba commented Jul 26, 2023

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

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

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

@Segu-g
Copy link
Member

Segu-g commented Jul 26, 2023

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

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

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

@Hiroshiba
Copy link
Member

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

@Segu-g
Copy link
Member

Segu-g commented Jul 29, 2023

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

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

return { commandChangeText, ...storeToRefs(textStore) };

@Hiroshiba
Copy link
Member

storeToRefs良いですね!!

@Hiroshiba
Copy link
Member

Hiroshiba commented Aug 5, 2023

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

  • お気持ち
    • stateをありえない状態にしてはいけない(テキストは無いのにaudio_queryはあるのは異常な状態)
      • ので、意味を持った変更単位にしたい
    • stateを気軽にさわれないようにしたい
    • stateのsetはちょっと目立つようにしておきたい
    • asyncの中で複数回stateを変更するのは良くない
    • 他のstoreのstateは直接変更できないようにしたい
    • 関心事が近いものは近いところで定義してほしい
  • ルール候補(ESLintでもOK)
    • stateの遷移はmutationで必ず定義
      • 何をしたいかの記録を義務付けられる
      • 異常な状態への遷移を避けられる
    • mutationはstateとpayloadを引数に持ち、asyncじゃないprivate関数
    • 1mutation内でできるのは与えられたstateの変更と複数mutationのみ
    • 1action内でできるのは複数action+複数mutation(+$patch)
      • stateは直接書き換えてはいけない
    • 他のstoreのmutation/patchは実行不可能
  • 問題点
    • mutationの中からactionを呼んでも気づけない
      • actionとmutationが区別できる仕組みがあるといいかもしれない
        • .actでaction、.mutでmutationが返すとか
        • 関数にするとか(?)
      • ESLintでなんとかなるかもしれない
      • actionはasyncにする
  • その他メモ
    • $patch(mutation)しなくてもmutation(state)でstate変更は可能っぽい
    • commandのmutationから普通のmutationをそこそこ簡単に叩けると良さそう
    • 普通のmutationからcommandのmutationは叩けなくてOK
    • ESLintで検査できると嬉しそう

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

  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
Copy link
Member

Segu-g commented Aug 5, 2023

上記のコメントを踏まえて少しサンプルを書いてみました。
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
Copy link
Member

Segu-g commented Aug 7, 2023

また別の話になりますが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
Copy link
Member

Hiroshiba commented Aug 7, 2023

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

  • 新しくこういうのも考慮できると最高だなという事項のメモ
    • 順不同に書けること
    • action/mutationの見た目的な区別が付きやすいこと
    • draftとstateでスコープが別れてること
    • 他のstateの情報が見れること
  • 新たな課題感
    • storeを超えて異常状態になることがありそう(loadProjectとかエンジンまわりとか?)
      • 割り切って良い気がする

アイデア1

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

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

アイデア2

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

アイデア3

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

アイデア4

prefixかsuffixにactmutを必須に。

@Hiroshiba
Copy link
Member

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

アイデア5

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

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

@Hiroshiba
Copy link
Member

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

@Hiroshiba
Copy link
Member

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

@Hiroshiba
Copy link
Member

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

@Segu-g
Copy link
Member

Segu-g commented Nov 11, 2023

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

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

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

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,
  };
});

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

@Segu-g
Copy link
Member

Segu-g commented Nov 12, 2023

インターフェースを改良しました.
asActgetRefの代わりにgetter.getmutation.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
Copy link
Member

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

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

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

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

  • Action型を返すdefActを作り、Action.dispatch経由で実行
    • actionであるということがわかりやすいように
    • こうしておくとmutation内でactionを実行されてしまう可能性をESLintで排除できる
  • Actions型やMutation型を外して全てただの関数にするunwrapStore的なのを作る
    • .vue内で使うのはこっち、.dispatchとかを付けなくても良いようになる
    • 他store内で使うのはunwrap前のやつ
    • ActionやMutationをブランド型(シンボルをプロパティにつけるやつ)にしておけば実装は容易
  • どのstoreが誰向けなのかわかるようにしておくといいかも
    • useHogeForVueとかuseHogeForStoreとか・・・?
  • 諦める点
    • 複数のstore.stateを一括変更できるグローバルなコマンド
      • そういうものはないという前提。。
      • 既存で存在すると再考しないといけないかも
    • 煩雑性
      • どのstoreをimportすれば良いのかとか
    • mutation内でactionを実行できてしまうこと
      • 将来はESLintで弾きたいが、しばらくはコードレビューで弾くことになる
      • .dispatch()と.commit()

@Hiroshiba
Copy link
Member

Hiroshiba commented Dec 4, 2023

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

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

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

コードを読んでいましたが、コマンドの中だとAudioItemを足すCOMMAND_REGISTER_AUDIO_ITEMと、消すCOMMAND_MULTI_REMOVE_AUDIO_ITEM辺りが簡単なように見えました。

COMMAND_REGISTER_AUDIO_ITEM: {

この2つだけとりあえず実装してみてプルリクを出していただく・・・という進め方はいかがでしょうか・・・!! @Segu-g

@Segu-g
Copy link
Member

Segu-g commented Dec 4, 2023

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

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

前との差分としては

  • defGet, defMut, defAct, defCmdなどの型を変更
    • 関数にプロパティを生やすのをやめ, getter, mutationは.funcで元の関数が得られる
    • getterは.get(), mutation.commit(), actionは.dispatch()でstateを注入した関数を呼べる
  • typescriptのreadonlyは安全ではないのでBrandを用いてreadonlyなstateをwritableなstateに代入できないように

です

@Hiroshiba
Copy link
Member

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

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

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

@Hiroshiba
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
優先度:低 機能向上 要議論 実行する前に議論が必要そうなもの
Projects
None yet
Development

No branches or pull requests

3 participants