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

refactor: エンジン情報のキャッシュを作らないように変更 #2272

Merged

Conversation

Hiroshiba
Copy link
Member

@Hiroshiba Hiroshiba commented Sep 25, 2024

内容

EngineInfoManagerで、エンジン情報のキャッシュを作らないようにしてみました。
これは毎回エンジン情報をディレクトリやファイルから取ってくるようになってしまいますが、キャッシュ由来のバグがなくなるのでこっちの方がいいかなぁと。

↓の関連issueのタスクでエンジン情報を何度か取得・登録を繰り返しそうなので、一旦変えてみました。
不要そうならまた後で戻してもいいかも。

関連 Issue

その他

調査してそもそもこうした方がいいと思った経緯はこんな感じです。

  • キャッシュを作ってる意味の調査

多分キャッシュを作るより、毎回ロードする方が分かりやすいはず。
fetchEngineInfosを毎回ロードするように変更して、代替ポート情報に合わせてhostを書き換えるようにした。

・・・という感じです!

@Hiroshiba Hiroshiba requested a review from a team as a code owner September 25, 2024 18:04
@Hiroshiba
Copy link
Member Author

Hiroshiba commented Sep 25, 2024

@sabonerune いつもすみません!!!
プルリクの変更が微妙そうだったらご指摘お願いできると嬉しいです 🙇 🙇 🙇
(これでいいのかだいぶ迷ったので・・・。)

あとこの辺りのリファクタリングってご興味あったりされませんか・・・?
調べていた時にこういう方針だったら代替ポートのバグを防げそうな気がしたので、一旦共有まで!!

このコメントを今のコードで解釈し直した感じです)

  • 動的に書き換わる情報と、書き換わらない情報を分けると良さそう
    • 本来エンジンURLのプロトコルとhostnameは書き換わらない情報のはず
    • 全部hostプロパティに含まれちゃってるのが課題、分けたい
    • 書き変えるのはaltPortInfoのみにしたい
  • そもそもなぜengineInfo.hostを書き換えているか
    • たしかポート変更を確実にエンジン側に伝えるため?
    • エンジン側にリクエストを飛ばしているのは一箇所に集約できているので、そこさえ間違えなければ大丈夫
  • やること一覧
    • engineInfo.portをengineInfo.defaultPortに変更する
    • engineInfo.hostを正しくhostだけ入ってるようにする
      • ここが一番厄介なはず、hostにurl全部が入っている前提のコードになってるので、いたる所を書き換える必要がある
    • Vuexのsrc\store\proxy.tsでengineInfo.hostを参照してる部分を、host+portを使うように書き換える
    • ついでにaltPortInfo.fromが不要になるはず

@sabonerune
Copy link
Contributor

多分大丈夫だと思います。

リファクタリングの件とりあえずやってみようと思います。

@Hiroshiba
Copy link
Member Author

Hiroshiba commented Sep 30, 2024

!! プルリク見てくださってありがとうございます!! 🙇 🙇
(このあたりわかる方が少なく、いつも頼ってしまってすみません。。)

リファクタリングの件とりあえずやってみようと思います。

おお、ぜひぜひ!!
draft PRとか気軽に作っちゃっていただければ、こっちからもなにかコメントできるかもです。
(この規模のUIソフトをメンテしたことがある人が少なく、僕含めみんな初心者だと思うので、気づきやアイデアを出し合うのがお互い役立つだろうなーと。)

@Hiroshiba
Copy link
Member Author

マージします!

@Hiroshiba Hiroshiba merged commit a8cd233 into VOICEVOX:main Sep 30, 2024
8 checks passed
@Hiroshiba Hiroshiba deleted the altPort周りのリファクタリング branch September 30, 2024 11:02
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