-
Notifications
You must be signed in to change notification settings - Fork 313
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
Fix: エンジンが起動していないと画面が真っ白になるのを修正 #1799
Fix: エンジンが起動していないと画面が真っ白になるのを修正 #1799
Conversation
src/sing/utility.ts
Outdated
return styleInfo.styleType === "humming" || styleInfo.styleType === "sing"; | ||
}; | ||
|
||
export const filterCharacterInfosByStyleType = ( |
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.
100%のsingではないので、別の所に移しても良さそう...?
あとこの関数で置き換えられるところが幾つかあるかも。
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.
PRありがとうございます!
こちらのPRですが、少し方向性を変えた方がよいかも...?と思いました。
問題の根幹はエンジンを起動していないとuserOrderedCharacterInfos
がundefined
になってエラーがthrowされることだと思います。
もともと使われていたGET_ORDERED_ALL_CHARACTER_INFOS
は、エンジンが起動していない場合に空のリストを返していたためにエラーが起きていなかったのだと思います。
私がUSER_ORDERED_CHARACTER_INFOS
とGET_ORDERED_ALL_CHARACTER_INFOS
の差異を理解していなかったために、ロジックが若干変わってしまった(ユーザーによる並び替えか、エンジンによる並び替えか)部分はあるので、戻してしまうのは良いと思ます。
ただ、今回のPRではコードとして似たロジックを増やす形になっています。
これを回避するために2つほど方法を提案してみます。
- コメントを付けるなどして、エラーハンドリングが行われることが正しくないことを明示したうえで、既存のロジックを活用して、
userOrderedCharacterInfos
がundefined
の場合は?? []
で受け止めて、PRの表題の問題が解消されるようにする。 - 似たロジックが増えることが問題なので、似たロジックをすべて新しく提案されたPR内のもの(
filterCharacterInfosByStyleType
)に置き換えてしまう。
PRの表題となる問題は、「エンジンが起動してない場合」つまりブラウザ版などで起きることで、製品として配布するものへの影響度は大きくないはずで、急ぎで修正が必要となるものではないと思います。
コードのメンテナンス性を上げるために、より良い選択が出来ればと思うのですが、どうでしょうか...?
Co-Authored-By: y-chan <[email protected]>
Co-Authored-By: y-chan <[email protected]>
修正しました。一緒にUnit Testも足しています。 |
どうして起きるのか理解できていなかったのですが、「エンジンが起動するまで」は「エンジンが起動していない」状態になってエラーが起きてしまうのですね...! unitテストも良いと思います!非常に助かりました! せっかくここまで実装していただいたので、提案させていただいた2の方針で行きたいのですが、時間がないので、私の方からPRを送らせていただきました。ご確認いただけると 🙏 あと、
について、もともとfilterはstoreでしていたものなので、 |
マージしました。 |
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.
レビューで見逃していました、ありがとうございます!!
ちょっとまだ時間的に余裕があるので(?)コメント書いてみました。
タイムリミットになったら申し訳ないですがマージさせていただこうと思います! 🙇
Co-authored-by: Hiroshiba <[email protected]>
学校などの関係でこれ以降の編集は厳しそうです。これ以降の編集はそちら側でお願いします、Allow edits from maintainersしてあるはずなのでpushできるはず? |
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.
ありがとうございます、引き継ぎます!
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.
たぶんできました!
多分大丈夫だと思うのでマージします! |
内容
タイトル通りです。
関連 Issue
(なし)
スクリーンショット・動画など
(なし)
その他
(なし)