VOICEVOX / voicevox

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

vuexのstoreの呼び出しから関数定義へコードジャンプできるようにする #2088

Open Segu-g opened 6 months ago

Segu-g commented 6 months ago

内容

現在のvoicevoxの状態管理は Vuex に独自のカスタムヘルパーを加えることで型サポートを受けています。 しかし現在のVuexの型を拡張したものでは、action, mutationの呼び出しが dispatch("ACTION1"), commit("MUTATION1") のような文字列での参照になってしまう為コードジャンプができません。

これを改善し xxx.ACTION1() のようにプロパティとして呼び出せることにすることでコードジャンプができるようになります。

Pros 良くなる点

コードジャンプができるようになり、開発体験が向上する。

Cons 悪くなる点

現在のあくまでVuexのサポートとしての型サポートと違い、 呼び出し側での構文が変わるので初心者が混乱する。

実現方法

文字列の関数渡しとプロパティアクセスの互換は Proxyを用いることで実現できる。

const xProxy = new Proxy({}, {
  get(target, tag: string) {
    return (arg) => console.log(`${tag} ${arg}`);
  }
});
xProxy.abc("def");
// "abc def" 

これを用いてvuexのkeyをプロパティアクセスから呼び出せるようにし、入力支援が効くようにする。

VOICEVOXのバージョン

0.?.0

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

その他

Segu-g commented 6 months ago

現在のgetter, mutaiton, actionを集約する記法のまま拡張するとすれば、 以下のようにredux風の書き方になると思います。

export const storeOptions = {
    ACTION1: {
        action({ dispatch }) {
            actions.ACTION2(dispatch, { x: 1 });
        }
    },
    ACTION2: {
        action(context, payload: { x: number }) {
            ...
        }
    }
};

export const actions = extractActions<typeof storeOptions>();

変更点としては

type.ts が無くなることで、メソッドを一覧として見るのが難しくなるため

export const { ACTION1, ACTION2 } = extractActions<typeof storeOptions>();

のようにしても良いかもしれません。

P. S. redux に近づけるなら dispatch(ACTION1(payload)) の方が綺麗かも...?

Hiroshiba commented 6 months ago

@Segu-g issue作成とまとめありがとうございます!! コードジャンプ可能になるのはかなり助かるので、ぜひ進めたいです!!

状態管理ライブラリについて思うところはいくつかありますが、優先度としては

  1. このissueの目的であるコードジャンプを可能にするのを最重要
  2. 次いで、その進行がスムーズに進んでいけると嬉しい
  3. あとは使いやすいと嬉しい

という順番かなと思いました!

2のために、一斉に全部置き換えなくて良い形だと嬉しいです・・・! 両方使える形にしておいて、ある程度の単位のファイル数を同時に置き換えていくのを想像しています。


Proxy使うのなるほどです、良さそう!

書式の方もなるほどです。 mutationもactionの兄弟に並ぶ感じでしょうか。それが可能だったら差分が少なくなるので良いなぁと。 あっでもACTION(dispatch, payload)になるのはちょっと意識しづらいかもですね・・・。

extractActionsなるほどです! これは必要なactionをimportする差分が発生してしまうかもです。まあコンフリクトは起こりにくそうですが。 getters・mutations・stateは全部store.でアクセス可能な一方、actionだけは個別にimportが必要になるな~と思いました。 reduxライクにするのが最高の設計かもしれませんが、一旦コードジャンプを叶える目的に着目するのもありかも・・・? (あと将来pinia化してstoreがいっぱいになったとき、どのactionがどのstore由来かわからなくなってしまうので、実は個別にACTIONをimportするのはなの不便かもとかちょっと思いました。)

目指したい書式ですが、今がこうなので

const store = useStore();

const audioKeys = computed(() => store.state.STATE);
const uiLocked = computed(() => store.getters.GETTER);
await store.dispatch("ACTION", payload);

こうできると直感的かも・・・?(実現難度を考えずに提案しています 🙇 )

await store.dispatch.ACTION(payload);
// あるいは
await store.actions.ACTION(payload);

できるのかな・・・。

dispatch.ACTIONactions.ACTIONのどっちが良いか迷ってます 😇 dispatch.ACTIONだとdispatchが呼ばれることは想像できるけど動詞.アクションになってなんか変。 actions.ACTIONだと変じゃないけどdispatchが呼ばれることを意識から外してしまいそうでちょっと問題あるかも

ただstore.dispatchにする場合、今のstore.dispatchと被っちゃうので、一旦別の値にする必要がありそう。 というのもあって、一旦お試してactionsにしても良いかも・・・?

将来設計のことも考えるとこうしたい、などあれば気軽に言っていただけると!!

Segu-g commented 6 months ago

優先度の件を見返した上で再度考えると、redux風の記法にするのはこのissueではなく別にやるべきですね! type.tsを残したまま、actionの引数及びstoreをProxyする形で呼び方だけを変える方法で実装したいと思います。 大体以下のような形になります。

// at src/component/*.vue
const store = useStore();
store.actions.RUN({ id: 1 });

// at src/store/*.ts
const storeOptions = createPartialStore<StoreTypes>({
    ...
    RUN: {
        mutation(state, payload: { id: number }) {
            ... 
        },
        async action({ state, actions, mutations }, payload: { id: number }) {
            mutaitons.RUN(payload),
            return await actions.HOGE();
        },
    },
    ...
});

dispatch.ACTIONかactions.ACTIONのどっちが良いか迷ってます 😇

actions.ACTIONの方が名前が衝突しない為、誤解を与えることが無くいいと思います👍 どちらにしてもvoicevoxのvuexは独自記法が一杯なので、変に類似操作を再定義するよりも別に定義した方が良いかと!

extractActionsなるほどです! これは必要なactionをimportする差分が発生してしまうかもです。まあコンフリクトは起こりにくそうですが。 getters・mutations・stateは全部store.でアクセス可能な一方、actionだけは個別にimportが必要になるな~と思いました。 reduxライクにするのが最高の設計かもしれませんが、一旦コードジャンプを叶える目的に着目するのもありかも・・・?

すみません!これは私の書き方が悪かったのですが、mutationやgetterも同様にimportしてredux風に使えるようにする想定でした!また、importに関しては import { audioActions } from "./audio.ts" のように纏めてexportしてもらう方法もあるので、import文が長くなることを心配しているならこのようにすればよさそうです。

(あと将来pinia化してstoreがいっぱいになったとき、どのactionがどのstore由来かわからなくなってしまうので、実は個別にACTIONをimportするのはなの不便かもとかちょっと思いました。)

逆に私はimportがそのままstore間の依存関係を示すことになるので、他storeのメソッドが暗黙的にcontextに渡ってくるよりもreduxのように明示的なimportを強制できる記法の方が良いと思っています。特にstoreを分割する際にはどれだけstoreがpureか、結合しているか把握することが重要であるので、そういった意味で依存一覧を確認できる手法はあった方が良いかと!

Hiroshiba commented 5 months ago

早速PRありがとうございます!!

ちょっとお手数おかけしてしまうのですが、時間あれば段取りチェックリストを作っておくと後で困らないかもと思いました! ざっくりですが「設計・ドット記法用のStore関数周り実装・PartialStoreの置き換え・既存Store関数周り置き換え」とか? もう前2つはほぼ終わってますが・・・!

もちろん変えていただいても問題ないです 🙏 例えばissueトップのその他のとこに書いてると見やすそうかも。


関数をそれぞれimportする形式、なるほどです! 個人的にはACTION等じゃなくstoreをimportしてstore.ACTION()などとする方が良いかなと思った感じです。 まあ、実際に書いてみないとわからないかもですね・・・!

Hiroshiba commented 4 months ago

@Segu-g 調子はどうでしょうか 👀 困っていることがあったら何でもコメントいただけると・・・!

結構変更量が多くなってしまってコンフリクトまみれになりそうなので、直近で動かないやつから順に置き換えていくのが良いかも・・・? ちょっとソングはマルチトラック周りもあるので後回しの方がありがたいかもです・・・! トークはちょっと空いてて狙い時かも。膨大だけど・・・。 例えばsetting.tsとか、ui.tsとか・・・?

Segu-g commented 4 months ago

@Hiroshiba 連絡が途切れてしまい申し訳ございません🙇‍♂️ ここのところ別の開発に没頭しておりこちらの方を放置してしまっていました。

状態といたしましては、とりあえず置き換えていくだけで、storeとuiのdispatch及びcommitを置換するだけなので困るようなところもございません。 ちょっとあまり差分が大きくなっても良くないので、今後は10ファイル程度単位でPRを出していこうと思います。

Hiroshiba commented 4 months ago

おお、ありがとうございます!! ちょっと時間空いてしまったのですがプルリク見させていただきます!!

Hiroshiba commented 3 weeks ago

@Segu-g 怒涛の変更ありがとうございました!!!!!

これでこのissueとしては完成でしょうか? .dispatch等の使わなくなった関数を消していく作業だけ・・・?

長きに渡る戦いが・・・!!!!!

Segu-g commented 1 week ago

すみません🙇返信が遅くなりました。 とりあえず使用している箇所の修正は以上になります。

元の型を変更するとするなら

  1. 'commit', 'dispatch'の型をデフォルトに戻す。
  2. 'commit', 'dispatch'を使えなくする。
  3. 'commit', 'dispatch'の型を今のまま残す。 の3通りが考えられ、 それぞの場合にdeprecatedタグを付けるかどうかの2択があります。 個人的には1番でdeprecatedにするのが良いと思うのですがどうでしょうか?
Hiroshiba commented 1 week ago

@Segu-g おお、ありがとうございます!!

deprecatedタグを付ける形の、1か2が良さそうに思いました!

個人的にはdispatchしようとした人が「あーこうすればいいんだな」とわかるのが重要だと思ってて、deprecatedをつけつつその関数ドキュメントでactions.ACTION名を使ってくださいと案内すれば行けそうだな~と。

1か2はどちらでも良さそうですが、Vuexに慣れてる方はとりあえず使える1のが良さそうに感じました! ただぶっちゃけ1にこだわる理由はないので、実装が楽なら2でも全然ありかなと・・・!!