Change: ブラウザ版のConfigをBaseConfigManagerベースに #1629
Conversation
fc50e9f to
ff3f473
Compare
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
| 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 | ||
| sed -i -e 's|"executionArgs": \[\],|"executionArgs": ["--port=50021"],|' .env.test | ||
| cp .env.test .env | ||
| sed -i -e 's|"../voicevox_engine/run.exe"|"${{ steps.download-engine.outputs.run_path }}"|' .env | ||
| sed -i -e 's|"executionArgs": \[\],|"executionArgs": ["--port=50021"],|' .env |
There was a problem hiding this comment.
色々試してたときのやつですね。問題自体はここには無かったのでいらないっちゃ要らないはず。
There was a problem hiding this comment.
なるほどです！
エンジンIDの変更が意図的かどうかパッとわからないので、やったほうが良さそうであればエンジンリポジトリと同様に環境変数へ、なくても良さそうであればこの行を消すはどうでしょう？
.env.testを書き換えるかはどちらでも良さそう。
（どっちかというと.envを自由に書き換えるほうがローカル変数っぽみがあって直感的だとは思います。.env.testは元からあるのでグローバルっぽみ。）
There was a problem hiding this comment.
Viteは.envより.env.testの方が優先度が高いので、.envと.env.testは同期しておきたいんですよね
There was a problem hiding this comment.
あ、たしかに！！気をつけないといけない仕様ですね･･･。
（だとしたらexecutionArgsが元の.env.testの空っぽのに上書きされてそうですけど、なんで今まで動いてたんだろう）
.env.test直接書き換えで良い気がしました！
src/browser/storeImpl.ts
Outdated
| async afterInitialize() { | ||
| if (this.config == null) { | ||
| throw new Error("assert: config != null"); | ||
| } | ||
| this.config.engineSettings[defaultEngineId] ??= engineSettingSchema.parse( | ||
| {} | ||
| ); | ||
| } |
There was a problem hiding this comment.
onupgradeneededに書くとconfigの初期化周りで処理が重複しそうだったのでフックを用意してそこに書きました。名前は変えても良いかも。
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
|
lockでいい感じに書き直してみました。 |
src/browser/storeImpl.ts
Outdated
| export async function withConfigManager<T>( | ||
| callback: (configManager: BrowserConfigManager) => Promise<T> | ||
| ): Promise<T> { |
There was a problem hiding this comment.
ここはどうしてもelectron側と実装が異なるので複雑性を1段階回すことになってしまうことに気づきました。
以下共有用メモです。
この辺り見てたんですが、
electron側はgetConfigManagerがasyncじゃなくて良い一方、
browser側はgetConfigManagerがasyncになる必要がありそうでした。
根本の課題は起動シーケンスの違いで、electronはバックエンドが最初なのに対し、browserはフロントエンドが最初なのでConfigManager初期化のタイミングがないんですよね･･･。
仮にバックエンドを初期化する関数を用意しても、Vueを初期化する前だとawaitできない･･･。
|
レビューを反映しました。 |
|
@yamachu さんもレビューありがとうございました！ |
内容
タイトル通り。
関連 Issue
スクリーンショット・動画など
（なし）
その他
（なし）