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

Disable @typescript-eslint/prefer-promise-reject-errors rule #389

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Mrtenz
Copy link
Member

@Mrtenz Mrtenz commented Feb 19, 2025

This disables the @typescript-eslint/prefer-promise-reject-errors rule, which enforces that Promise.reject must be called with an instance of Error. This rule seems too strict, especially in cases when working with try-catch for example:

new Promise((resolve, reject) => {
  try {
    return resolve(doSomething());
  } catch (error) {
    return reject(error);
  }
});

In this case we could wrap the error in new Error(), but if it's already an Error, we would loose information like the stack trace etc., so we'd need to add more code to try to handle the case where error is an Error, or a string, object.

if (error instanceof Error) {
  return reject(error);
}

if (isJsonRpcError(error)) {
  return reject(serializeJsonRpcError(error));
}

return reject(new Error(error));

While this rule seems useful in some cases, I feel like it's too strict for us.

@Mrtenz Mrtenz marked this pull request as ready for review February 19, 2025 13:13
@Mrtenz Mrtenz requested review from a team as code owners February 19, 2025 13:13
@Gudahtt
Copy link
Member

Gudahtt commented Feb 19, 2025

To clarify, are you saying that it's too strict because we'd be forced to verify that the error is an instance of Error?

Surely we're not intentionally throwing non-Errors. That's what I saw as the benefit of this rule - it's an additional check to ensure we're not throwing a non-Error.

If handling unknown is the only concern, there is an option for that: https://typescript-eslint.io/rules/prefer-promise-reject-errors/

@Mrtenz
Copy link
Member Author

Mrtenz commented Feb 19, 2025

To clarify, are you saying that it's too strict because we'd be forced to verify that the error is an instance of Error?

Yes. It was causing a lot of errors in Snaps at least.

Surely we're not intentionally throwing non-Errors. That's what I saw as the benefit of this rule - it's an additional check to ensure we're not throwing a non-Error.

If handling unknown is the only concern, there is an option for that: https://typescript-eslint.io/rules/prefer-promise-reject-errors/

Good point. I wasn't aware of that option, that seems like a good alternative to completely disabling the rule.

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

Successfully merging this pull request may close these issues.

2 participants