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のstoreの呼び出しから関数定義へコードジャンプできるようにする #2088

Open
1 of 8 tasks
Segu-g opened this issue May 19, 2024 · 8 comments
Open
1 of 8 tasks

Comments

@Segu-g
Copy link
Member

Segu-g commented May 19, 2024

内容

現在の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の種類/ディストリ/バージョン

  • Windows
  • macOS
  • Linux

その他

  • dot access storeの設計・実装
  • commandの改修
  • UI Actionのstore改修
  • 全store移行
  • コンポーネント移行
@Segu-g
Copy link
Member Author

Segu-g commented May 19, 2024

現在の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 からの離脱
  • 呼び出し方式の変更
  • 型定義のインライン化
    などが挙げられます。

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

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

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

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

@Hiroshiba
Copy link
Member

Hiroshiba commented May 21, 2024

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

Segu-g commented May 22, 2024

優先度の件を見返した上で再度考えると、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
Copy link
Member

Hiroshiba commented May 26, 2024

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

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

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


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

@Hiroshiba
Copy link
Member

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

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

@Segu-g
Copy link
Member Author

Segu-g commented Jul 13, 2024

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

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

@Hiroshiba
Copy link
Member

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

@Hiroshiba
Copy link
Member

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

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants