algolia / algolia/algoliasearch-client-javascript

Throw real Errors instead of objects

Open
#1,444 3 comments 0 reactions 0 assignees View on GitHub
enhancement
Dominant language
TypeScript
Stars
1.4k
Forks
226
PR merge metrics
No merged PRs in 30d

Description

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.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with packages/client-account/src/methods/accountCopyIndex.ts and packages/client-account/src/errors/createDestinationIndiceExistsError.ts, then inspect the other error helpers referenced by the issue. Decide on the consistent Error representation and verify that thrown failures preserve their names, messages, and real stack traces for consumers such as Sentry.

Written by the indexing model from the issue text.

Assessment

Tech stack
typescript
Domain
api
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
35/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.