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

Vue3の<script setup>を使うようにする #1065

Closed
Hiroshiba opened this issue Dec 25, 2022 · 25 comments · Fixed by #1180 or #1241
Closed

Vue3の<script setup>を使うようにする #1065

Hiroshiba opened this issue Dec 25, 2022 · 25 comments · Fixed by #1180 or #1241

Comments

@Hiroshiba
Copy link
Member

内容

VOICEVOXはVue3のComposition APIを使ってコンポーネントを定義しています。
これは1つ大きな課題があります。

  • templateで利用する変数・関数をreturnに全部書く必要がある
    • この影響でEditorHomeなどは数十個のreturnが必要になっています
    • また初学者にとってわかりづらいという問題もあります
    • コードジャンプするときにreturnしている方へのジャンプも表示されるという課題もあります(VSCode+Volar)

この問題は<script setup>を使うと解決できます。
https://v3.ja.vuejs.org/api/sfc-script-setup.html

emit/propの書き方が変わって、その型定義に若干の修正が必要ですが、問題ない程度だと思います。

Pros 良くなる点

returnが不要になる。
setup()関数のネストが減る。

Cons 悪くなる点

書き直しが必要

実現方法

https://v3.ja.vuejs.org/api/sfc-script-setup.html

@Hiroshiba
Copy link
Member Author

@MT224244 さん、 @y-chan さん、あと @k-chop さん辺りにもどんな感じか感想もらえると心強いです!!

@Hiroshiba
Copy link
Member Author

📝 IDEとの相性は要チェック

@sevenc-nanashi
Copy link
Member

image
image

VSCode、Neovimでは補完が聞きました(Volar使用)

@Hiroshiba
Copy link
Member Author

おお、良さそうですね!!

@MT224244
Copy link
Contributor

少し気になるのは、

definePropsdefineEmits は、<script setup> 内でのみ使用可能なコンパイラマクロです。インポートする必要はなく、<script setup> が処理されるときにコンパイルされます。

と謳っているのにTypeScriptがインポートを求めてくるところですね。
image
// @ts-ignore でエラーを無視すると動いたので、型定義をTypeScriptが見つけられていないだけのようです。
まぁ普通にインポートしても動いたのでそんなに気にするほどの事ではないかもしれません。
(ついでに、ドキュメントでは defineEmits なのに何故か実際に定義してあるのは defineEmit でした。)

もう一つ、試しにいくつかのコンポーネントを<script setup>に書き換えてみたのですが、こんな警告が出ていました。

[Vue warn]: setup() return property "$q" should not start with "$" or "_" which are reserved prefixes for Vue internals.

どうやら $ _ で始まる変数名は宣言しない方がいいようですね。
これも変数名を変えるだけなので問題にはならないとは思いますが、一応報告しておきます。

@Hiroshiba
Copy link
Member Author

調査ありがとうございます!!
import求められるのは謎ですね。ふと思ったのはVueとかそこ周りのバージョン違いだったり・・・?

$qがwarningなのはなるほどです。
そもそも多言語だと一般的じゃない気がするので、quasarとかQとかquとかに変えちゃうのが良いかなと思いました!

@sevenc-nanashi
Copy link
Member

defineEmitはVueのバージョン違いっぽいです。Vueを3.2.45(最新版)にしたらdefineEmitsになりました。

@sevenc-nanashi
Copy link
Member

image
また、declareが追加されたのか、LSPがエラーを吐かないようになりました

@k-chop
Copy link
Contributor

k-chop commented Dec 26, 2022

  • script setupはvue@3.2からの機能らしいので、まずバージョンを上げる必要がありそう
  • componentsを明示的に書かなくてよくなったり、propsの型がtypescriptの記法で書けるのすばらしい 👏
  • <script setup> のトップレベルで定義した変数が全部templateに公開されるので、もしかするとどこかで名前衝突したりがあるかも(ないかも)
  • 全体的にメリットの方がでかそう

この手の移行は機械的にできるcodemod(ASTベースでコード変換するやつ)が既にあるのでは?
と思って調べてたんですが、見つけられなかったので今試しに書いてみています 👀 (手で書いた方が早い説もあります)

@Hiroshiba
Copy link
Member Author

詳細にありがとうございます!!
名前衝突はありそうですね。inputとかに代入するとDOMのinput変わっちゃうとか…?(流石になさそうですが)

@y-chan
Copy link
Member

y-chan commented Dec 26, 2022

script setup構文ですが、私の使っているWebStormや、VS Codeでも、IDEとの相性は特に問題なさそうでした...!

JetBrains系、VS Code、neovimで問題ないようで、開発体験を大きく損なうようなことは起きないはずなので、書き換えしちゃっていいと思います...!

@Hiroshiba
Copy link
Member Author

検証ありがとうございます!!

では、割と積極的に置き換えていく方針で進めていきたいと思います!!

@Hiroshiba
Copy link
Member Author

Hiroshiba commented Dec 26, 2022

置き換えていく方針ですが、おそらく凄まじい変更量になると思います。
でも大多数はケバブケース→パスカルケースの置き換えと、setup()関数のインデントが2つ下がるだけなはずです。
(変更がインデントだけの行のdiffは、githubの機能の「Hide white space」で非表示にできます)

パッと思いつく方法は2つあります。

1. えいやで全部変える

というPRを作ってくださったのが #1069 です。
差分4500行は凄まじいので、別の方法を取りたいです。

(VOICEVOXは過去2回こういうことを経験したのですが、どちらも1箇所以上でデグレーションがありました)

2. ファイルをちょっとずつ変更していく

±が合計500くらいなら意外と見れます。
今回は単調なので±800くらいでも大丈夫そうです。

これで進めるとかなり嬉しいのですが・・・どうでしょうか・・・?
たぶん、一部のファイルをaddしてPR出す、というのを5~8回くらいやる感じになるかなと思っています。

@k-chop
Copy link
Contributor

k-chop commented Dec 27, 2022

よさそうに思います!

よりレビュー負荷を下げるなら、ひとまず各ファイル「scriptタグ内の変更のみに絞る」という方法もありそうです。
コンポーネント名のPascalCaseへの書き換えはとりあえず置いといても動くので。

defineProps / defineEmits が単純置き換えではないので、そこ集中してチェックできるようにしたいですね。
required を見て型をoptionalにする必要があったり、 withDefault が結構書き方ややこしかったり...)

残ったPascalCaseへの書き換えは単純置換のみになるはずなので、最後に全ファイル一括でやれるかも...?

@sevenc-nanashi
Copy link
Member

置き換えた人から:
パラメータはほぼ全部requiredでした、optionalはErrorBoundary(だっけ?)くらいしかなかったはずです

@Hiroshiba
Copy link
Member Author

よりレビュー負荷を下げるなら、ひとまず各ファイル「scriptタグ内の変更のみに絞る」という方法もありそうです。

こちらもできるとより良いと思います!!
もしGithub上でワーニングが出まくるようであれば、最初からケースも変更しないとかなと思いました!


他のアイデアとして別ブランチで作業することも考えたのですが、これはmainにマージする際にコンフリクトが凄いことになりそうなのでやめといた方がいいかなと思いました。

@Hiroshiba
Copy link
Member Author

Hiroshiba commented Jan 2, 2023

ということで、

・ファイル単位でscript内を置き換えていく
 ・変更量見てある程度一括でもOK
・template内の置き換えは↑でワーニングが出るかどうかで分岐
 ・出ないなら放置して最後に一括で修正
 ・出るなら一緒に直す

って感じですかね…!!

@sevenc-nanashi @k-chop @MT224244 この作業やってってもいいよって方いらっしゃいませんか…!

@k-chop
Copy link
Contributor

k-chop commented Jan 2, 2023

方針了解です!

方針決まっていくつか修正箇所はあると思いますが、せっかく #1069 で置き換えしてくださっているので
@sevenc-nanashi さんに #1069 を小分けにしていって頂いてもいいのかなという気がしました

自分がやるのでも全然大丈夫です 🙋

@sevenc-nanashi
Copy link
Member

script setup化のやつをファイルごとに出すように分けたいと思いますー。
キャメルケースは後で。

@sevenc-nanashi
Copy link
Member

大量のマージコミットが生まれちゃいそうなので、to-script-setupみたいなブランチを生やしてそこにPRしていく形の方が良いかも…?

@k-chop
Copy link
Contributor

k-chop commented Jan 2, 2023

ありがとうございます 🙇

5~8本くらいのPRで行けそうという話だったので
そのくらいなら1個ずつmainにマージしていっても問題ないのかなと思いました。
(参考情報: #1069 の差分がhide whitespaceして +1558 −3052

今回のscript setup化の影響は各コンポーネント内に閉じているので新旧混在してても特に問題なく、逐次mainに取り込んでいった方が最後のコンフリクト解消の手間も減りそうかなと思います!

@Hiroshiba
Copy link
Member Author

ですね、コンフリクト解消がすごいことになるかもなので、別ブランチ切り出しはやめたほうが良さそうって感じでいます!

#1065 (comment) 引用

他のアイデアとして別ブランチで作業することも考えたのですが、これはmainにマージする際にコンフリクトが凄いことになりそうなのでやめといた方がいいかなと思いました。

@Hiroshiba
Copy link
Member Author

そういえばこちらのタスクまだ途中かもです。マルチエンジンが一段落したし再開しても良いかも・・・?

@sevenc-nanashi
Copy link
Member

sevenc-nanashi commented Feb 4, 2023

雑にチェックリストを生成:

  • App.vue
  • components/AcceptRetrieveTelemetryDialog.vue
  • components/AcceptTermsDialog.vue
  • components/AudioAccent.vue
  • components/AudioCell.vue
  • components/AudioDetail.vue
  • components/AudioInfo.vue
  • components/AudioParameter.vue
  • components/CharacterButton.vue
  • components/CharacterOrderDialog.vue
  • components/CharacterPortrait.vue
  • components/ContactInfo.vue
  • components/DefaultStyleListDialog.vue
  • components/DefaultStyleSelectDialog.vue
  • components/DictionaryManageDialog.vue
  • components/EngineManageDialog.vue
  • components/ErrorBoundary.vue
  • components/FileNamePatternDialog.vue
  • components/HeaderBar.vue
  • components/HeaderBarCustomDialog.vue
  • components/HelpDialog.vue
  • components/HelpPolicy.vue
  • components/HotkeySettingDialog.vue
  • components/HowToUse.vue
  • components/LibraryPolicy.vue
  • components/MenuBar.vue
  • components/MenuButton.vue
  • components/MenuItem.vue
  • components/MinMaxCloseButtons.vue
  • components/OssCommunityInfo.vue
  • components/OssLicense.vue
  • components/PresetManageDialog.vue
  • components/ProgressDialog.vue
  • components/QAndA.vue
  • components/SaveAllResultDialog.vue
  • components/SettingDialog.vue
  • components/TitleBarButtons.vue
  • components/ToolTip.vue
  • components/UpdateInfo.vue
  • views/EditorHome.vue

@sevenc-nanashi
Copy link
Member

(間違えてcloseつけちゃってた…)

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