Require an Error object when rejecting a promise

[fabiomcosta]

Rejecting without an Error is a really bad pattern that can be easy to miss.
catch code should not have to expect an undefined or null error.

https://eslint.org/docs/rules/prefer-promise-reject-errors

I also went ahead and made no-throw-literal an error. throwing a string literal, or anything that is not an Error makes debugging a horrible experience, because there is no stack.

So these 2 are very related and should not only be discouraged, but considered errors.

https://eslint.org/docs/rules/no-throw-literal

[3rd-Eden]

How does this rule deal with libraries that argument error objects like err or even custom error objects?

[3rd-Eden]

I agree that promise.reject() should always have an arguments, but forcing that argument to always be a Error is a step to far IMHO.

[fabiomcosta]

@3rd-Eden custom error types should work just fine.
One of the examples at https://eslint.org/docs/rules/prefer-promise-reject-errors is using new TypeError().
Given eslint can't really trace type information, my guess is that it just makes sure the argument is not a literal string, or null or undefined (to the best it can).

https://eslint.org/docs/rules/prefer-promise-reject-errors

This also confirms what I'm saying: https://eslint.org/docs/rules/prefer-promise-reject-errors#known-limitations

[3rd-Eden]

Did you actually test these rules or did you make the assumption that they work in the way you wanted them to work? I took some time today to specify these rules in an example project to ensure that they would not trigger on valid cases such as custom errors and enhanced error objects. But during this testing I noticed that these rules are only triggered when you as developer intentionally types null or undefined:

throw null;
throw undefined 

When you, as developer explicitly type of those values I would assume that there is a valid reason for it, and it should be allowed. The things that you do care about it when you accidentally throw a null or undefined.

This however does not trigger the lint rule:

const x;

throw x;

new Promise(function (resolve, reject) {
  reject(x);
})

The examples above are things you can to get caught, but are ignored by your proposed rules. Instead if only triggers on examples where developers explicitly set null, or undefined as values. Given that need to be typed out, I would assume that this is a developers intent, and not something that we should prevent. Now the only valid thing that it would trigger on is when you forget to supply a argument. That is an issue that tis not specific to errors, or promises and is not something that should be solved by a linter, but by a type checker instead.

So given this in-depth analysis for the actual rule and how it works with actual code I have give a -1 on this addition.

[DullReferenceException]

I'd offer that omitting something from throw/reject or throwing a non-Error is typically the wrong thing to do. If there was a reason a developer decides to do it anyway, they can always disable the rule on that line. Throwing things like strings as errors cause complications/bugs for things like global error handling, so this seems like a good use of a linter (flagging probable bugs). Of course type constraints are better, but because so many JS developers shy away from using types, it's useful to have linting rules as a fallback.

[fabiomcosta]

Did you actually test these rules or did you make the assumption that they work in the way you wanted them to work?

Yes, we are using this on our project.

When you, as developer explicitly type of those values I would assume that there is a valid reason for it

The no-throw-literal is mostly intended to protected against throwing string literals, as the name suggests, which won't provide any stack trace information and will be hard to debug.
throw null; and throw undefined; are just other cases that it also throws because also have the same issue.
Can you give me one valid case where a developer would want to throw null;?

So given this in-depth analysis for the actual rule and how it works with actual code I have give a -1 on this addition.

Thank you for your input.

So to summarize, these rules are mostly intended to protect against the 2 following sort of common and mostly accidental cases:

throw 'this error';
// and
Promise.reject();

[gingur]

Regarding https://eslint.org/docs/rules/no-throw-literal, the purpose of these eslint rules is to define standards and although there might be edge cases where someone would want to throw null, I don't see anything wrong with saying as a standard we don't promote such syntax. When those edge cases come up, they can always add an eslint ignore comment if they must like @DullReferenceException mentioned.

Regarding https://eslint.org/docs/rules/prefer-promise-reject-errors, I see it as a good standard to provide an error object over any other format, however I would suggest we relax the setting to allowEmptyReject: true, allowing the error object to be optional.

[gingur]

suggest relaxing this new rule by adding allowEmptyReject: true to allow Promise.reject() to still work, but Promise.reject("no") to error

[gingur]

actually, after thinking about it a bit more, disregard. the following code would still produce a catch w/ undefined which I would not want. so even Promise.reject should always be called with an error object.

async function example() {
  return Promise.reject();
}
(async () => {
  try {
  	const res = await example();
  } catch (e) {
    console.log('err', e); // undefined
  }
})();

[indexzero]

Great discussion 🤟