[Improve code]: astro/src/components/Client/bsky/* 手動formatter・Linter対応 #83
Conversation
Deploying skyshare with
|
| Latest commit: |
703a463
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ccb210d6.skyshare.pages.dev |
| Branch Preview URL: | https://preview-issue-55.skyshare.pages.dev |
| @@ -221,25 +225,31 @@ export const Component = ({ | |||
| ) | |||
| // Blobのアップロードに失敗したファイルが一つでも存在した場合停止する | |||
| resultUploadBlob.forEach(value => { | |||
| if (typeof value?.error !== "undefined") { | |||
| if ("error" in value && typeof value.error != "undefined") { | |||
There was a problem hiding this comment.
この箇所 value.error != "undefined" が厳格な比較じゃなくなったのって何か理由がありますでしょうか。
（そもそもinで判定してるから要らない可能性はありえますが...）
There was a problem hiding this comment.
@nkte8
レビューありがとうございます！
理由としては次の2つでしょうか
- JSONの定義1には
null型が存在し、atproto APIのレスポンスとしてnullが入り得るため- 要検証: レスポンスの
messageが未定義で、{"code": "123"}などであった場合、TypeScriptではmessage: undefinedと見做される可能性もあり、undefinednullの両方が入りえると考えられます
- 要検証: レスポンスの
- 現状ではレスポンスの型定義が
typeof JSONのため、undefinednullの両方が入りうることを見据えて
ただし、TypeScript側で型定義をしている場合であれば、厳密等価演算子=== !==を用いる方針で統一したほうが良いかもしれませんね...
関係のない箇所も等価演算子(== !=)に書き直してしまった気がするので、そちらに関しては後々、元のコードに修正します。
また質問なのですが、"error" in valueのみの形式で、エラー型を判定する方針で統一しても問題ないでしょうか？
そのようにした場合、その後の成功レスポンスをas MyTypeの形式でアサーションする必要がなく、後々の修正が楽になるかと思います
（val as MyTypeや<MyType>valで型アサーションした変数は、実際の型に関わらず、アサーションした型として見做されてしまうので、可能であれば減らしたい。という考えです）
TypeScriptとAPIの型定義に関する議論は、こちらの記事が参考になるかもしれません
Footnotes
There was a problem hiding this comment.
@ZEKE320
説明・根拠等の提示ありがとうございます！
- これは確かにありそうですね。レスポンスとして定義されていない変数が返された際に
nullとundefinedどちらになるのかわからない問題。一方、現状のAtprotoの仕様においてはエラーの場合は必ずerror,messageを返すようになっているため、この懸念が必要ないのではないか、とも思っています(https://atproto.com/specs/xrpc#error-responses)。 typeof JSONのところはおっしゃる通りですね（そのままでいいと思いました）
typeof JSONではない箇所については気にする必要ないかなと思いました（現在仕様が入り乱れてしまっているので恐縮ですが..）
ただし、TypeScript側で型定義をしている場合であれば、厳密等価演算子=== !==を用いる方針で統一したほうが良いかもしれませんね...
そうですね。typeof JSONについてはすべて削除したいと思っていて (#88 )、厳密透過演算子で統一したいという気持ちがあります。atproto_apiについては #15 の仕様によっては、判定処理も必要ないかもしれませんが...（調査中です）
また質問なのですが、"error" in valueのみの形式で、エラー型を判定する方針で統一しても問題ないでしょうか？
そのようにした場合、その後の成功レスポンスをas MyTypeの形式でアサーションする必要がなく、後々の修正が楽になるかと思います
（val as MyTypeやvalで型アサーションした変数は、実際の型に関わらず、アサーションした型として見做されてしまうので、可能であれば減らしたい。という考えです）
私は現状のATProtoの仕様状況も踏まえて、errorがResponseに帰ってくるのはエラーの時だけである、という前提があるので"error" in valueのみの形式で、エラー型を判定するでいいと思っていますが、上で提示していただいた(1)(2)以外にも懸念事項ある感じでしょうか（良ければ教えていただければ幸いです🙇）
There was a problem hiding this comment.
@nkte8
返信ありがとうございます！このほか特に問題はありません
確認となりますが、
オブジェクトのユニオン型の取り扱い
エラーを意味する特定のキー(errorなど)を含むユニオン型は、そのキーの有無("キー名" in response)のみで型判定する。（このとき、特定のキーが存在した時点でエラーとみなし、値の型については確認しない）- 1により、
ESLintTypeScriptではユニオン型に関するエラーが解消するため、
今回のPRで追加された型アサーション付き変数(val as Typeや<Type> val)は削除する。
変数のnull undefined型の取り扱い
- 変数に明確な型定義が存在する場合、
undefinednullに対しても厳密等価演算子(===,!==)を用いて型判定を行う
という方針で問題ありませんか？
一方、現状のAtprotoの仕様においてはエラーの場合は必ずerror,messageを返すようになっているため、この懸念が必要ないのではないか、とも思っています(https://atproto.com/specs/xrpc#error-responses)。
atprotoの仕様共有ありがとうございます！
余談ですが、
JSONの定義1には
null型が存在し、atproto APIのレスポンスとしてnullが入り得るため
そもそも{"error": null}のような謎レスポンスを返すとは考えにくいので、null型のケースあまり考えなくても良いかもしれませんね笑
もちろん、"error" in valueのみで判定する方針であれば、この点については心配ないと思います
There was a problem hiding this comment.
@ZEKE320 方針の整理ありがとうございます！認識合います。 #84 #84 (comment) の方も基本的に同じ方針でOKです、PRの方に細かいこと書いておきます。
|
（@ZEKE320 対応終わりましたら Re-request review を押していただけると判断付くのでお願いします！ ） |
|
@nkte8 |
* 🚨 astro/src/utils/atproto_apiに`eslint . --fix`を実行 * 🎨 astro/src/utils/atproto_api/models/*にPrettierを適用 * 🎨 astro/src/utils/atproto_api/*にPrettierを適用 * ✏️ searchの誤字を修正 * 🏷️ imageの$typeを"blob"のみに限定 * 🚨 暫定的に、astro/src/utils/atproto_api以下のレスポンスを、as構文で型付け * 🎨 astro/src/utils/atproto_api/*.tsにPrettierを適用 * 🏷️ uploadBlobの戻り値の型情報を分割 * [Improve code]: astro/src/components/Client/bsky/* 手動formatter・Linter対応 (#83) * 🚨 astro/src/components/Client/bsky/にESLintの--fixオプションを適用 * 🔒️ target="_blank"付のタグの脆弱性対策を追加 詳細は下記を確認してください。 https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-no-target-blank.md * 🚨 readDrafts()が常にtruthyな値を返すようなので、OR演算子を削除 * 🏷️ atproto_apiの型定義変更に伴い、既存の実装の型情報を更新 * 🚨 Promise<void>を返す関数にvoidキーワードを追加 * 🚨 バッククォートで囲われたダブルクォートのエスケープを解除 * ✏️ identifierの綴りを修正 * 🚨 map()で繰り返し出力されるコンポーネントに、key属性を追加 * 🚨 使用されないエラー変数にアンダースコアを追加 * 🏷️ atproto_apiの型定義変更に伴い、既存の実装の型情報を更新 * 🎨 astro/src/components/Client/bskyにPrettierを適用 * ✏️ `LoginForm`の注意文言に、環境変数の`servicename`を用いるよう変更 * 💬 エラー処理後の型アサーションを解説するコメントを追記 * 🥅 使用されていないエラー変数名をアンダースコアのみに変更 * 🚨 エラー型の判定を`"error" in val`形式で統一 * ♻️ 変数定義と値の代入を凝集化 * 🚨 エラー型の判定を`"error" in val`形式で統一 * 🏷️ 型アサーションを用いない実装に変更 * 🔥 不要になったimport文を削除 * 🎨 astro/src/components/Client/bsky/*にPrettierを適用 * ESLint修正漏れの修正 --------- Co-authored-by: nekono <[email protected]>
* [Improve code]: pages/* 手動formatter・Linter対応 (#77) * 🚨 未使用の引数`error`の名前にアンダースコアを追加 * 🚨 テンプレートリテラル内のURL型をstring型に変換 * 🎨 astro/src/pagesにPrettierを適用 * 🎨 astro/src/components/*.astroにPrettierを適用 (#76) * 🎨 astro/src/layoutsにPrettierを適用 (#80) * [Improve code]: utils/atproto_api/* 手動formatter・Linter対応 (#81) * 🚨 astro/src/utils/atproto_apiに`eslint . --fix`を実行 * 🎨 astro/src/utils/atproto_api/models/*にPrettierを適用 * 🎨 astro/src/utils/atproto_api/*にPrettierを適用 * ✏️ searchの誤字を修正 * 🏷️ imageの$typeを"blob"のみに限定 * 🚨 暫定的に、astro/src/utils/atproto_api以下のレスポンスを、as構文で型付け * 🎨 astro/src/utils/atproto_api/*.tsにPrettierを適用 * 🏷️ uploadBlobの戻り値の型情報を分割 * [Improve code]: astro/src/components/Client/bsky/* 手動formatter・Linter対応 (#83) * 🚨 astro/src/components/Client/bsky/にESLintの--fixオプションを適用 * 🔒️ target="_blank"付のタグの脆弱性対策を追加 詳細は下記を確認してください。 https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/jsx-no-target-blank.md * 🚨 readDrafts()が常にtruthyな値を返すようなので、OR演算子を削除 * 🏷️ atproto_apiの型定義変更に伴い、既存の実装の型情報を更新 * 🚨 Promise<void>を返す関数にvoidキーワードを追加 * 🚨 バッククォートで囲われたダブルクォートのエスケープを解除 * ✏️ identifierの綴りを修正 * 🚨 map()で繰り返し出力されるコンポーネントに、key属性を追加 * 🚨 使用されないエラー変数にアンダースコアを追加 * 🏷️ atproto_apiの型定義変更に伴い、既存の実装の型情報を更新 * 🎨 astro/src/components/Client/bskyにPrettierを適用 * ✏️ `LoginForm`の注意文言に、環境変数の`servicename`を用いるよう変更 * 💬 エラー処理後の型アサーションを解説するコメントを追記 * 🥅 使用されていないエラー変数名をアンダースコアのみに変更 * 🚨 エラー型の判定を`"error" in val`形式で統一 * ♻️ 変数定義と値の代入を凝集化 * 🚨 エラー型の判定を`"error" in val`形式で統一 * 🏷️ 型アサーションを用いない実装に変更 * 🔥 不要になったimport文を削除 * 🎨 astro/src/components/Client/bsky/*にPrettierを適用 * ESLint修正漏れの修正 --------- Co-authored-by: nekono <[email protected]> * [Improve code]: src/utils/*.ts 手動formatter・Linter対応 (#78) * 🚨 browser-image-compressionエラー処理のリンティングエラーを修正 * 🎨 astro/src/utils/*.tsにPrettierを適用 * ✏️ searchの綴りを修正 * ✨ useLocalStorage.tsにzodによる型チェックを実装 * 🏷️ Zodオブジェクトをタグ、下書きに各々定義 * ➕ 依存関係にZodを追加 * 🏷️ Zodオブジェクトから型情報を定義 & 実際の使用方法に即した型名に変更 * [Improve code]: lib/* 手動formatter・Linter対応 (#84) * 🚨 astro/src/libにESLintのfixオプションを適用 * 🚨 Astroの環境変数をstring型とみなす記述を追加 * 🚨 nullish判定を明確化 * 💬 コメントの空白を半角に変更 * 🏷️ 変数の型情報を明記 * 🎨 astro/src/libにPrettierを適用 * ✏️ ファイル名getIdの綴りを修正 * ✏️ ファイル名変更に伴いimport文を修正 * 🏷️ Astro API用の型をZodで再定義 * 🦺 APIレスポンス取得箇所にバリデーションチェックを追加 * 🏷️ モジュール外で型の名前が重複しないよう変更 * 🩹 APIレスポンス処理時の型エラーに関する文言を修正 * 🏷️ API呼び出し箇所の型定義更新に伴い、エラー判定箇所を修正 * 🥅 getOgpMetaエラー時のステータス番号を定義 * ✏️ Zodオブジェクトの命名規則を統一 * 🥅 エラーレスポンスからhtmlを削除 * 🔥 使用されていないレスポンス型定義用のJSONを削除 * 🥅 エラー型判定の実装を改善 * 🥅 エラー型の判定を`"error" in val`形式で統一 * ⚰️ 使用されていない変数を削除 * ⚰️ 不要になった行を削除 * ➕ 依存関係にZodを追加 * 🥅 エラー型の判定ロジックを変更 & 型アサーション削除 * 🔥 不要になったインポート行を削除 * API応答を待機するコードを追加 (#92) * 🔥 ヘッドブランチを`preview/*`に制限するコードを削除 * 🎨 変数の定義順と使用順を統一 * 💬 分かりやすいコメントに変更 * ✨ ベースブランチに`hotfix/*`が指定できるように修正 --------- Co-authored-by: nekono <[email protected]>
Resolves #55
以下のPRによる変更を前提に修正しています。
本PRのマージする場合、上記PRのレビューもお願いします。
本PRで行われた変更
target="_blank"付のタグの脆弱性対策を追加 (40f97ec)readDrafts()の戻り値がstring[]でfalsy値になることが無いようなので、空の配列を代入する処理を削除 (624cf9e)Promise<void>の関数使用箇所にvoidキーワードを追加 (c8c9587)identifierの綴りを修正 (fc6aca6)map()で繰り返されるコンポーネントにkey属性を追加 (d5221f3)atproto_apiの型仕様変更に伴い、既存の実装の型情報を更新 (82ad2cd, ac8f1d7)Prettierを適用 (7c1baac)