-
Notifications
You must be signed in to change notification settings - Fork 309
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
Vueファイル内の型エラーを減らす #1295
Vueファイル内の型エラーを減らす #1295
Conversation
ちょこっと調査してみたところ、 export type MenuItemCheckbox = MenuItemBase<"checkbox"> & {
- checked: ComputedRef<boolean>;
+ checked: boolean;
onClick: () => void;
disabled?: boolean;
disableWhenUiLocked: boolean;
}; にすると解決するっぽい感じでした(Vue詳しくないんだけど、これなんでなんですかね…) で、現時点 MenuBar component では checkbox を使用していないようです |
未使用であれば一旦消してしまっても良いのかなと思いました! |
再度実装する際computedじゃない値でも出来ないか試してみると良さそう
# ~/Projects/github.com/yamachu/voicevox $ [type-strict]
npm run typecheck:vue-tsc
> [email protected] typecheck:vue-tsc
> vue-tsc --noEmit
現時点の状態だと型エラー排除完了 🚀 GitHub Actionsのtest workflowに追加すると project-s ブランチのテスト周りが通らなくなるんじゃないかなと思い、追加するのを躊躇しています… |
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!
helperのテストもしっかり整備していただいてありがとうございます...!:pray:
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です!!
VOICEVOXのコードはわりとnullとundefinedが混じってしまっているので、undefinedだけでなくnullableを想定したほうが良いかも?とちょっと思いました。どうでしょう。。
(undefinedToDefault → nullableToDefault みたいな)
うーん、これに関しては分けた方がいい気がします というのもundefinedはたとえばMapから引っ張ってきてなかった、とか、そもそも定義されてなかったみたいなケースで使って、nullって明示的にこの状態をnullにしている、っていう時を想定して使う…と思うんですね(このprojectだとどういう扱いにする、みたいの明示していたか記憶にないので、実体験から書いています そうやって表現したい状態が違うものを同じ関数で扱うようにすると、例えば途中でnullを返す処理が入った時にdefault値が返ったりするわけですが、それはnullの時に表現したいものだったのか?と考えなくてはならなそうなので… |
Co-authored-by: Hiroshiba <[email protected]>
Co-authored-by: Hiroshiba <[email protected]>
undefinedやnullについての扱いどうしようか悩んでいるIssueがあったのでメモがてらはっておこう |
なるほどです。プロジェクトとしてどちらも許容されているのが良くないなと思いました! |
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!!
ref: #1293
の続きをやってみました
vue-tsc
を上げればそもそも対応しなくて良いコードも出るのではという仮説のもと上げてみた所、MenuItem
のonClick
が対応不要になったため、エラーを減らす文脈としてvue-tsc
のバージョンアップも併せて行っています内容
で出力されるエラーをより減らします
関連 Issue
スクリーンショット・動画など
14a4f76
の対応で上記のエラーも解決
その他
satisfies
を使ってみようかなと TS 4.9 を入れてみたら更にエラーが増えました…メモ: FIXMEがついているMenuItem.vueのgetMenuBarHotkeyの箇所