Skip to content
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

Throw real Errors instead of objects #1444

Open
alexandre-butynski opened this issue Mar 9, 2023 · 3 comments
Open

Throw real Errors instead of objects #1444

alexandre-butynski opened this issue Mar 9, 2023 · 3 comments

Comments

@alexandre-butynski
Copy link

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 and all the informations about the real error are lost.

We do this kind of thing all over the package:

accountCopyIndex.ts

throw createDestinationIndiceExistsError()

createDestinationIndiceExistsError.ts

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

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

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

Or even to create custom error objects

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.

@Haroenv
Copy link
Contributor

Haroenv commented Mar 9, 2023

I agree that we should return real errors, but changing this could be a breaking change if anyone relies on the type or the keys of the object now. @shortcuts, in v5 we are throwing real errors right?

@alexandre-butynski
Copy link
Author

I agree that it can be considered as a breaking change. Looks good to me if it's fixed in the next major release 👍

@psavan1655
Copy link

Hi, i would like to work on this issue, as i have good knowledge of throwing proper errors. So i will be able to handle it properly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants