Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat: ErrorCauseへの対応 #1732
feat: ErrorCauseへの対応 #1732
Changes from 6 commits
a25f234
6e67e69
b5521ac
71702ee
6b446ed
b2ed1a7
5a537e2
8afc210
5b8e17e
795d5f3
8635497
644fc46
da723f6
aba5852
91e7b42
5fa0a56
699cc31
5f1f1af
89bb9de
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
という感じですかね。
Original Errorというのを何というかちょっと難しい。。
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.
散らかすようですが「本来のエラー」という言い方もできそうです。
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.
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.
エラーの発生元?とかですかね? ( 実はcauseはなんでも入れられるのでなんでもアリだと思いますが)
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.
の観点を考えると L320-L321丸々削ってしまってもいいのではないかと思いました。
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.
📝 スクリーンショットは、後でここにまとめて撮り直す。
https://github.com/asciidwango/js-primer/blob/master/source/basic/error-try-catch/screenshot-manual.sh
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.
これ、手元にmacOSマシンがなくて実行できなさそうでした 🙇
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.
スクショは統一感が大事だと思うので(OSでUIが異なる)、一旦そのままで大丈夫ですー
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.
error
自体のログにcauseを含むスタックトレースが入ってるので、明示的にcauseを参照する必要性はないかなと思います。エラーキャッチする側が
.cause
が存在しているかを知ってるのはおかしいと思うので、console.error(error.cause)
と書くケースは実際にはないかなとは思っています。(
.cause
はオプショナルなので、全てのエラーに存在するわけじゃないため、何かしら判定して出すかブラウザ側に任せるのが正しい気はします)まだFirefoxだけだったのか。。(意外)
Node.jsは独自で対応してたのか。
ChromeもCanary版ではサポートしてたので、
.cause
は省略していいかなーとは思いました。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.
承知しました!削除します