-
Notifications
You must be signed in to change notification settings - Fork 304
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: ポート変更時にEngineInfo
の情報を書き換えないようにする。
#2282
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です!!!!!
完璧です、ありがとうございます!!!
ちょっと細かい部分でコメントさせていただきました!
もしよければ!
RuntimeInfoの方にも代替ポート情報渡さないとなの抜けていました。。。。
ありがとうございます 🙇 🙇 🙇
@@ -105,7 +105,7 @@ export type Command = { | |||
export type EngineState = "STARTING" | "FAILED_STARTING" | "ERROR" | "READY"; | |||
|
|||
// ポートが塞がれていたときの代替ポート情報 | |||
export type AltPortInfos = Record<EngineId, { from: number; to: number }>; | |||
export type AltPortInfos = Record<EngineId, string>; |
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.
(ただのコメントです)
仮にdefaultPortがundefinableになっても、ここは必ずstringになりそう!
const engineHostInfo: HostInfo = { | ||
protocol: engineInfo.protocol, | ||
hostname: engineInfo.hostname, | ||
port: Number(engineInfo.defaultPort), |
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.
HostInfo.port、Numberじゃなくてstringでも良さそうな気がしますね!
まあとはいえEngineManifestのportがnumberなので全部stringにはできないですね。
まあどこからstringにするかというだけですが、このエディタ上では大体stringで統一しておくと良さそうかも?
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.
portHelper周りがNumberで処理しているのでここがNumberとStringを切り替える部分だと思いました。
メモ: portHelper周りをStringに書き換える場合portが数字であることを確認しないとOSコマンドインジェクション脆弱性を引き起こすかもしれない。
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.
なるほどです!!
portHelper周りもstringで良い気もしました!
けどまあ、既存コードになじませる意図だとNumberのが良さそうなので、このプルリクエストはこの形が良さそうだなと感じました!!
ということでresolve!
メモ: portHelper周りをStringに書き換える場合portが数字であることを確認しないとOSコマンドインジェクション脆弱性を引き起こすかもしれない。
現状、engineManifestの読み込みは型のバリデーションも含むzodで行っているので、必ず数値しか入らないと思われます。
ちょっとこのプルリクとは外れますが、危ないのは事実ですね・・・。
一番危ないのはたぶんコマンド実行している部分execFileSyncがshell: true
で動いてることだと思います。hostnameとかでいたずらされるかもしれない。
shell: false
にすれば問題ないはず・・・。
色々書いていますがあまり変更に気乗りじゃなかったらご連絡ください!! 🙏 |
ちょっとURLAPIの仕様確認に手間取っています。 |
Co-authored-by: Hiroshiba <hihokaruta@gmail.com>
一通り修正しました。 |
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!!
完璧なリファクタリングありがとうございました!!
もしよかったらまたぜひ力をお借りしたいです・・・!(このPR内でも出てたshell: trueをfalseにしていくとか)
リファクタリング協力してもいいよ〜って感じでしたら、またお願いさせていただいても良いでしょうか 🙇
ちょっとmain.ts
の解体をしていければとか思っているけど手が足りず・・・。
次はテキストファイルの受け渡しをしている部分だけをmain.tsから切り出す、とかどうでしょう・・・?
ここでimportして
voicevox/src/backend/electron/main.ts
Lines 32 to 41 in a1ace78
import { | |
ContactTextFileName, | |
HowToUseTextFileName, | |
OssCommunityInfosFileName, | |
OssLicensesJsonFileName, | |
PolicyTextFileName, | |
PrivacyPolicyTextFileName, | |
QAndATextFileName, | |
UpdateInfosJsonFileName, | |
} from "@/type/staticResources"; |
ここで読んで
voicevox/src/backend/electron/main.ts
Lines 252 to 299 in a1ace78
// 使い方テキストの読み込み | |
const howToUseText = fs.readFileSync( | |
path.join(__static, HowToUseTextFileName), | |
"utf-8", | |
); | |
// OSSコミュニティ情報の読み込み | |
const ossCommunityInfos = fs.readFileSync( | |
path.join(__static, OssCommunityInfosFileName), | |
"utf-8", | |
); | |
// 利用規約テキストの読み込み | |
const policyText = fs.readFileSync( | |
path.join(__static, PolicyTextFileName), | |
"utf-8", | |
); | |
// OSSライセンス情報の読み込み | |
const ossLicenses = JSON.parse( | |
fs.readFileSync(path.join(__static, OssLicensesJsonFileName), { | |
encoding: "utf-8", | |
}), | |
) as Record<string, string>[]; | |
// 問い合わせの読み込み | |
const contactText = fs.readFileSync( | |
path.join(__static, ContactTextFileName), | |
"utf-8", | |
); | |
// Q&Aの読み込み | |
const qAndAText = fs.readFileSync( | |
path.join(__static, QAndATextFileName), | |
"utf-8", | |
); | |
// アップデート情報の読み込み | |
const updateInfos = JSON.parse( | |
fs.readFileSync(path.join(__static, UpdateInfosJsonFileName), { | |
encoding: "utf-8", | |
}), | |
) as UpdateInfo[]; | |
const privacyPolicyText = fs.readFileSync( | |
path.join(__static, PrivacyPolicyTextFileName), | |
"utf-8", | |
); |
ここでGET関数を定義してますが、
https://github.com/VOICEVOX/voicevox/blob/a1ace7836753ecdb2b2dea0f304383642e668776/src/backend/electron/main.ts#L546C3-L580
全部読み込んでobjectを返し、あとは読み出し関数を作れば100行くらい減る気がしています。
こんな感じのkey型作って
type AssetTextType = "Contact" | "HowToUseText" | ...
あとはtextAsset.tsなどを作って(名前は適当)↓の関数作って
function loadTextAsset() {
return {
"Contact": ...,
"HowToUseText": ...,
}
}
main.tsでconstに代入しておく。
でipc関数にGET_HOW_TO_USE_TEXT
などがいっぱい生えてるので、ひとまとめに
GET_ASSET_TEXT: (textType: AssetTextType) => {
}
とか定義して・・・みたいな!
結構typescirptの型パズルがややこしい気がしてます。
難しそうだったら適当にdisableしちゃってdraftPR作っていただくとかで・・・・!!!
もしよければ・・・!!!
テキストファイルの受け渡しの方をやってみます。 |
おーぜひぜひ!!! |
内容
#2272 (comment) で提案されたリファクタリングです。