algolia / algolia/algoliasearch-client-javascript

Throw real Errors instead of objects

オープン
#1,444 コメント 3 件 リアクション 0 件 担当者 0 名 GitHub で見る
enhancement
主要言語
TypeScript
スター
1.4k
フォーク
226
PR マージ指標
30日以内にマージされた PR はありません

説明

This package has a non standard behavior of throwing objects instead of real JS Errors. As it works approximately in the same way in most cases and these objects are similar to Error (at least, TS is ok with that), there are some edge cases where it messes everything up.

My specific use case is with Sentry, which is not able to handle properly these "errors". Instead of having a proper stack trace, I have the Sentry error [Non-Error exception captured](https://sentry.zendesk.com/hc/en-us/articles/360057389753-Why-am-I-seeing-events-with-Non-Error-exception-or-promise-rejection-captured-with-keys-using-the-JavaScript-SDK-) and all the informations about the real error are lost.

We do this kind of thing all over the package:

[accountCopyIndex.ts](https://github.com/algolia/algoliasearch-client-javascript/blob/3aec216a9704f818b4faaa8b97d988ec2794f9da/packages/client-account/src/methods/accountCopyIndex.ts#L33)

```JS
throw createDestinationIndiceExistsError()
```

[createDestinationIndiceExistsError.ts](https://github.com/algolia/algoliasearch-client-javascript/blob/master/packages/client-account/src/errors/createDestinationIndiceExistsError.ts)

```JS
export function createDestinationIndiceExistsError(): Error {
return {
name: 'DestinationIndiceAlreadyExistsError',
message: 'Destination indice already exists.',
};
}
```

I propose instead to either throw a basic Error with the message...

```JS
export function createDestinationIndiceExistsError(): Error {
return new Error('DestinationIndiceAlreadyExistsError');
}
```

Or even to create custom error objects

```JS
class AlgoliaDestinationIndiceAlreadyExistsError extends Error {
constructor(message) {
super(message);
this.name = 'AlgoliaDestinationIndiceAlreadyExistsError';
}
}

export function createDestinationIndiceExistsError(): Error {
return new AlgoliaDestinationIndiceAlreadyExistsError('Destination indice already exists.');
}
```

I even think that this helper methods are not necessary and that custom errors could be thrown directly. For instance, the custom message is useless in this case.

I can open a PR with a proof of concept if you think it's a good idea. I would also be glad to hear your ideas about the right implementation.

コントリビューションガイド

このリポジトリのコントリビューションガイドは索引されていません

調査の方向性

packages/client-account/src/methods/accountCopyIndex.ts と packages/client-account/src/errors/createDestinationIndiceExistsError.ts から始め、その後、issue で参照されている他のエラーヘルパーを調査します。一貫した Error の表現を決定し、throw された失敗が Sentry などのコンシューマーに対して、その名前、メッセージ、実際のスタックトレースを保持していることを検証します。

索引モデルが issue の本文から書いたものです。

評価

技術スタック
typescript
領域
api
issue の種類
バグ
難易度
4/5
見積もり時間
3〜5日
活発さ
停滞
明瞭さ
おおむね明確
初心者へのやさしさ
35/100

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。