feat: ErrorCauseへの対応 #1732

himanoa · 2024-03-24T08:13:34Z

概要

Issue対応:
fix #1714

例外処理のセクションの最後に Error Cause の解説を追加します
Promiseを活用するのセクションのサンプルコードで Error Cause を使うように変更します

重点的に確認してもらいたい部分

Error Causeのセクションにおいて、causeプロパティで原因のエラーを取得できる部分の解説をコードブロック内に閉じ込めてしまったが、ちゃんと前の文章で表現したほうが良いか
causeプロパティにアクセスして console.error を呼び出す部分で詳細という単語を使っているか妥当かどうか。
- 他に考えられるとすると原因要因 etc...

Preview

https://deploy-preview-1732--js-primer.netlify.app/basic/error-try-catch/

TODO

コードの出力結果のスクリーンショットの最新化 feat(error): add screenshot for Error Cause #1734
埋め込みのREPLだとcauseのスタックトレースが表示されない REPLエディタがError Causeに対応していない #1735

See. asciidwango#1714

bot-user · 2024-03-24T08:14:28Z

✅ Deploy Preview for js-primer ready!

Name	Link
🔨 Latest commit	`89bb9de`
🔍 Latest deploy log	https://app.netlify.com/sites/js-primer/deploys/66040e0aab3b7f0008d4a404
😎 Deploy Preview	https://deploy-preview-1732--js-primer.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

azu · 2024-03-24T08:26:53Z

source/use-case/ajaxapp/promise/README.md

@@ -82,6 +82,7 @@ function main() {
        .catch((error) => {
            // Promiseチェーンの中で発生したエラーを受け取る
            console.error(`エラーが発生しました (${error})`);
+            console.error(`詳細 (${error.cause})`);


error 自体のログにcauseを含むスタックトレースが入ってるので、明示的にcauseを参照する必要性はないかなと思います。
エラーキャッチする側が.causeが存在しているかを知ってるのはおかしいと思うので、console.error(error.cause) と書くケースは実際にはないかなとは思っています。
(.causeはオプショナルなので、全てのエラーに存在するわけじゃないため、何かしら判定して出すかブラウザ側に任せるのが正しい気はします)

Cause is displayed in console
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause

まだFirefoxだけだったのか。。(意外)
Node.jsは独自で対応してたのか。

ChromeもCanary版ではサポートしてたので、.causeは省略していいかなーとは思いました。

Support error.cause in Chrome DevTools [40182832] - Chromium

承知しました！削除します

source/use-case/ajaxapp/promise/README.md

azu · 2024-03-24T08:29:19Z

source/basic/error-try-catch/img/error-cause.png

📝 スクリーンショットは、後でここにまとめて撮り直す。

https://github.com/asciidwango/js-primer/blob/master/source/basic/error-try-catch/screenshot-manual.sh

このシェルスクリプトは僕の環境で実行してコミットにスクリーンショット付ける形で大丈夫ですか？

これ、手元にmacOSマシンがなくて実行できなさそうでした 🙇

スクショは統一感が大事だと思うので(OSでUIが異なる)、一旦そのままで大丈夫ですー

Error: スクリーンショットを撮り直す #1733

source/basic/error-try-catch/README.md

azu · 2024-03-24T09:07:19Z

source/basic/error-try-catch/README.md

+エラーをキャッチし新しいメッセージや別の `Error` オブジェクトで送出すると、デバッグに有益な情報をエラーに付与することできて便利です。  
+これを実現する時に、新しく `Error` オブジェクトを作成して `throw` すると元のエラーのスタックトレースが失われてしまいます。
+
+この問題を解決するには、`catch` 句で補足した変換元の例外を、新しい `Error` オブジェクトのコンストラクタに渡すことで、変換元のエラーのスタックトレースを保持することができます。


このスタックトレースが失われる問題を解決するには、ES2022で追加されたErrorの`cause`オプションが利用できます。新しいエラーオブジェクトを作成する際に、第2引数の`cause`オプションに元々?のエラーオブジェクトを渡すことで、のスタックトレースを保持できます。

という感じですかね。
Original Errorというのを何というかちょっと難しい。。

元々のエラー

キャッチしたエラー

補足したエラー

補足したエラーかキャッチしたエラーが、指している対象が明確になっていて良いと感じました。

このファイルの一貫性重視でキャッチしたエラーを使おうと思います

散らかすようですが「本来のエラー」という言い方もできそうです。

本来のエラーは結構しっくり来るので、その方針にしようとおもいます！

source/basic/error-try-catch/README.md

azu · 2024-03-24T09:15:17Z

source/basic/error-try-catch/README.md

+}
+
+// 数値の文字列を受け取り数値を返す関数
+// 'text' など数値にはならない文字列を渡された場合は例外を送出する


Suggested change

// 'text' など数値にはならない文字列を渡された場合は例外を送出する

// 'text' など数値にはならない文字列を渡された場合は例外を投げる

source/basic/error-try-catch/README.md

azu · 2024-03-24T09:18:49Z

source/basic/error-try-catch/README.md

+} catch (err) {
+    console.error(`エラーが発生しました (${err})`);
+    // `cause` プロパティを参照することで、throwする時に `cause` で渡したエラーにアクセスすることができる
+    console.error(`詳細 (${err.cause})`);


エラーの発生元?とかですかね? ( 実はcauseはなんでも入れられるのでなんでもアリだと思いますが)

Suggested change

console.error(`詳細 (${err.cause})`);

console.error(`エラーの発生元 (${err.cause})`);

発生元というとどこで投げられたのかという「場所」のイメージをもたせそうですがそれはスタックトレースの情報なので、「原因」のニュアンスが与えられるほうがいいんじゃないかと思います。

(.causeはオプショナルなので、全てのエラーに存在するわけじゃないため、何かしら判定して出すかブラウザ側に任せるのが正しい気はします)
#1732 (comment)

の観点を考えると L320-L321丸々削ってしまってもいいのではないかと思いました。

azu · 2024-03-24T09:22:51Z

@himanoa 新しい文章が増える形になるのでContributor License Agreement（CLA）へのサインお願いします 🙏

https://docs.google.com/forms/d/e/1FAIpQLSfysn5ZMYZvk3VGE5flQ9AKxQszmP0cVQar1KnokrRtcrYSVQ/viewform

See. asciidwango#1732 (comment) Co-authored-by: azu <azu@users.noreply.github.com>

lacolaco

一通り読みました！

source/basic/error-try-catch/README.md

lacolaco · 2024-03-24T11:00:54Z

source/basic/error-try-catch/README.md

+} catch (err) {
+    console.error(`エラーが発生しました (${err})`);
+    // `cause` プロパティを参照することで、throwする時に `cause` で渡したエラーにアクセスすることができる
+    console.error(`詳細 (${err.cause})`);


発生元というとどこで投げられたのかという「場所」のイメージをもたせそうですがそれはスタックトレースの情報なので、「原因」のニュアンスが与えられるほうがいいんじゃないかと思います。

Co-authored-by: Suguru Inatomi <suguru.inatomi@gmail.com>

Co-authored-by: azu <azu@users.noreply.github.com>

himanoa · 2024-03-24T11:41:53Z

一旦修正方法が自明な部分だけ直しました

source/basic/error-try-catch/README.md

himanoa · 2024-03-26T01:24:58Z

承知です！修正します！

Co-authored-by: azu <azu@users.noreply.github.com>

azu

コメントしたところ以外はLGTM

source/basic/error-try-catch/README.md

azu · 2024-03-26T01:39:09Z

source/basic/error-try-catch/README.md

+}
+```
+
+この問題を解決するには、`catch` 句で補足した本来のエラーを、新しい `Error` オブジェクトのコンストラクタに渡すことで、スタックトレースを引き継ぐことができます。


#1732 (comment)
ここで書いたように cause オプションに渡すというのを文章で説明する必要があると思います。
そうしないとcauseオプションがいきなりコードに出てきてしまってるので、驚き最小の原則の反してしまう。
(コードを読む前に、コードに書かれている想像できるような説明を書くイメージ)

なるほどー修正します！

Co-authored-by: azu <azu@users.noreply.github.com>

azu

https://deploy-preview-1732--js-primer.netlify.app/basic/error-try-catch/

流れで読んでちょっと気になった部分をコメントしました。
他は大丈夫そうです。

azu · 2024-03-26T23:54:57Z

source/basic/error-try-catch/src/error-cause/index.js

+// 数字の文字列を二つ受け取り、合計を返す関数
+function sumNumStrings(a, b) {
+    try {
+        const aNumber = safeParseInt(a);
+        const bNumber = safeParseInt(b);
+        return aNumber + bNumber;
+    } catch (e) {
+        throw new Error("Failed to sum a and b", { cause: e });
+    }
+}
+
+// 数値の文字列を受け取り数値を返す関数
+// 'text' など数値にはならない文字列を渡された場合は例外を投げられる
+function safeParseInt(numStr) {
+    const num = parseInt(numStr, 10);
+    if (Number.isNaN(num)) {
+        throw new Error(`${numStr} is not a numeric`);
+    }
+    return num;
+}


safeParseIntとsafeParseIntは順番逆にした方が良い気はします。
既知 → 未知の順番にしたいので。

source/basic/error-try-catch/src/error-cause/index.js

Co-authored-by: azu <azu@users.noreply.github.com>

azu

LGTM!

ありがとうございました！

himanoa · 2024-03-27T14:30:05Z

@azu @lacolaco
手厚いレビューありがとうございました～！ 🙌

himanoa added 6 commits March 24, 2024 16:23

feat: add a error-cause section to error-try-catch

a25f234

See. asciidwango#1714

chore: add a error-cause to error-try-catch conclusion section

6e67e69

chore: Use cause property in ajaxapp/promise example codes

b5521ac

chore: access to cause property in Error cause section's example code

71702ee

refactor: Exec npm run textlint:fix

6b446ed

chore: align with other heading expressions

b2ed1a7

azu reviewed Mar 24, 2024

View reviewed changes

azu requested a review from lacolaco March 24, 2024 09:12

azu reviewed Mar 24, 2024

View reviewed changes

chore: change from active form to passive form

5a537e2

See. asciidwango#1732 (comment) Co-authored-by: azu <azu@users.noreply.github.com>

lacolaco reviewed Mar 24, 2024

View reviewed changes

himanoa and others added 4 commits March 24, 2024 20:26

chore: add conjunction

8afc210

Co-authored-by: Suguru Inatomi <suguru.inatomi@gmail.com>

chore: 表記ゆれの統一(送出 -> 投げられる)

5b8e17e

Co-authored-by: azu <azu@users.noreply.github.com>

chore: cut out a example code

795d5f3

fix: remove unnecessary console.error

8635497

himanoa added 2 commits March 26, 2024 00:06

fix: Error Causeの説明をより丁寧にする

644fc46

fix: remove logging cause property

da723f6

himanoa requested review from azu and lacolaco March 25, 2024 15:12

fix: exec npm run textlint:fix

aba5852

azu mentioned this pull request Mar 25, 2024

Error: スクリーンショットを撮り直す #1733

Closed

1 task

azu reviewed Mar 25, 2024

View reviewed changes

source/basic/error-try-catch/README.md Show resolved Hide resolved

fix: broken test

91e7b42

Co-authored-by: azu <azu@users.noreply.github.com>

azu reviewed Mar 26, 2024

View reviewed changes

chore: Remove empty lines

5fa0a56

Co-authored-by: azu <azu@users.noreply.github.com>

fix: add cause description

699cc31

himanoa requested a review from azu March 26, 2024 09:43

azu reviewed Mar 26, 2024

View reviewed changes

himanoa and others added 2 commits March 27, 2024 21:13

fix: use Number.parseInt instead of parseInt

5f1f1af

Co-authored-by: azu <azu@users.noreply.github.com>

chore: change function definition order safeParseInt and sumNumStrings

89bb9de

himanoa requested a review from azu March 27, 2024 12:16

azu approved these changes Mar 27, 2024

View reviewed changes

azu merged commit 76dd4c1 into asciidwango:master Mar 27, 2024
8 checks passed

azu mentioned this pull request Mar 27, 2024

feat(error): add screenshot for Error Cause #1734

Merged

azu added Type: Feature Lang: ES2022 ECMAScript 2022 labels Mar 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ErrorCauseへの対応 #1732

feat: ErrorCauseへの対応 #1732

himanoa commented Mar 24, 2024 •

edited by azu

bot-user commented Mar 24, 2024 •

edited

azu Mar 24, 2024 •

edited

himanoa Mar 24, 2024

azu Mar 24, 2024

himanoa Mar 24, 2024

himanoa Mar 25, 2024 •

edited

azu Mar 25, 2024 •

edited

azu Mar 24, 2024

himanoa Mar 24, 2024

lacolaco Mar 24, 2024

himanoa Mar 25, 2024

azu Mar 24, 2024

azu Mar 24, 2024 •

edited

lacolaco Mar 24, 2024

himanoa Mar 24, 2024

azu commented Mar 24, 2024 •

edited

lacolaco left a comment

lacolaco Mar 24, 2024

himanoa commented Mar 24, 2024

himanoa commented Mar 26, 2024 via email

azu left a comment

azu Mar 26, 2024 •

edited

himanoa Mar 26, 2024

azu left a comment

azu Mar 26, 2024 •

edited

azu left a comment

himanoa commented Mar 27, 2024

	// 'text' など数値にはならない文字列を渡された場合は例外を送出する
	// 'text' など数値にはならない文字列を渡された場合は例外を投げる

	console.error(`詳細 (${err.cause})`);
	console.error(`エラーの発生元 (${err.cause})`);

feat: ErrorCauseへの対応 #1732

feat: ErrorCauseへの対応 #1732

Conversation

himanoa commented Mar 24, 2024 • edited by azu

概要

重点的に確認してもらいたい部分

Preview

TODO

bot-user commented Mar 24, 2024 • edited

✅ Deploy Preview for js-primer ready!

azu Mar 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himanoa Mar 25, 2024 • edited

Choose a reason for hiding this comment

azu Mar 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azu Mar 24, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azu commented Mar 24, 2024 • edited

lacolaco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himanoa commented Mar 24, 2024

himanoa commented Mar 26, 2024 via email

azu left a comment

Choose a reason for hiding this comment

azu Mar 26, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azu left a comment

Choose a reason for hiding this comment

azu Mar 26, 2024 • edited

Choose a reason for hiding this comment

azu left a comment

Choose a reason for hiding this comment

himanoa commented Mar 27, 2024

himanoa commented Mar 24, 2024 •

edited by azu

bot-user commented Mar 24, 2024 •

edited

azu Mar 24, 2024 •

edited

himanoa Mar 25, 2024 •

edited

azu Mar 25, 2024 •

edited

azu Mar 24, 2024 •

edited

azu commented Mar 24, 2024 •

edited

azu Mar 26, 2024 •

edited

azu Mar 26, 2024 •

edited