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

engine-infos alias poc #2254

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Conversation

cm-ayf
Copy link
Contributor

@cm-ayf cm-ayf commented Aug 27, 2024

内容

VITE_DEFAULT_ENGINE_INFOを環境変数からJSON文字列で指定していたが,これの型を起動時に検証したくないから,環境変数で指定するのではなく.tsファイルに書いて,どの.tsを読むのかをvite.config.mtsで指定するようにした.

その他

とりあえずnpm run electron:serveだけは動くことを確認しました.他も追って確認します.

@cm-ayf
Copy link
Contributor Author

cm-ayf commented Aug 27, 2024

TODO: engine-infos/test.tsとしてどのようなものが commit されるべきで,GitHub Actions としてどのように変更すべきであるか確認する

sed -i -e 's|"074fc39e-678b-4c13-8916-ffca8d505d1d"|"208cf94d-43d2-4cf5-abc0-9783cac36d29"|' .env.test
sed -i -e 's|"../voicevox_engine/run.exe"|"${{ steps.download-engine.outputs.run_path }}"|' .env.test
# GitHub Actions 環境だとたまに50021が封じられていることがあるので、ランダムなポートを使うようにする
PORT=$(node -r net -e "server=net.createServer();server.listen(0,()=>{console.log(server.address().port);server.close()})")
sed -i -e 's|"host": "http://127.0.0.1:50021"|"host": "http://127.0.0.1:'$PORT'"|' .env.test
sed -i -e 's|"executionArgs": \[\],|"executionArgs": ["--port='$PORT'"],|' .env.test
cp .env.test .env

Copy link
Member

@Hiroshiba Hiroshiba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

おー!!!!
なるほど〜〜〜 .d.tsに型書けば良いのかぁ。なるほどです。

全テスト通すには結構いろいろ調整が必要かもです、不明な点あればなんでも聞いていただければ!!!
といってもvite周りは不慣れなので、わかるかわかりませんが・・・

いくつかこうできると嬉しいな〜〜〜って点を思いついたのでコメントさせていただきました!!!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

あ、.developmentの方はcommitしないようにしてたりします! 🙇
(個々の環境で変更するものをgit管理すると大変なので)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ちなみにenv全部まるまる移動することとかってできたりしますか・・・?
特に環境変数じゃないといけないような実装はしてないはずなのと、全部型付けられて幸せそうというのと・・・。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

もし可能ならrootディレクトリに直接.tsファイルを置けると嬉しい・・・!!
env.production.tsみたいな感じとかできれば、管理しないといけないファイル数減らせるよな〜〜〜と。

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

src/vite-env.d.tsでも良いかも?

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

Successfully merging this pull request may close these issues.

2 participants