-
Notifications
You must be signed in to change notification settings - Fork 312
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の呼び出しをリテラル引数からDot記法へ #2099
vuexのstoreの呼び出しをリテラル引数からDot記法へ #2099
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ほぼLGTMです!!
良いですね!!!
僕的にはコードの観点からダメそうなとこがわからないので、もうマージしても良いのではと感じました!
TypeScriptにちょっと明るそうな方にメンション飛ばさせていただきます 🙇
Proxyや大量の関数のreduceなどがありえますが、なにか問題点とか気づいたらいつでもコメントいただけると!!!
@sevenc-nanashi @yamachu @MT224244
自分はコードを一通り眺めて、違和感あるとこやドキュメントほしいとこにコメント書きました!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!!!
コードジャンプ素晴らしいですね!!
しばらく他の方のコメントがないか待ってみて、みんな忙しそうだったらとりあえずマージして進めてみる形がいいのかなと思ってます!
戻すのも難しくないはずですし・・・!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@yamachu レビューありがとうございます!! マージします!!! |
内容
store.ts内における関数の呼び出しにおいて、
dispatch("ACTION1", payloads)
のような引数におけるリテラル指定からactions.ACTION(payload)
のようにdot記法によるアクセスに変更します。とりあえずどのくらいの規模でPRを分割すべきかわからなかったので、元のコードは残し、
dictionary.ts
だけ新しい記法に変えています。type.tsは残したままなので lsp上で
goto definition
をしてもtype.ts
に移動するだけですが、goto implementation
で実装に飛べます。関連 Issue
ref #2088
スクリーンショット・動画など
その他