Suggestion: make no-throw more restrictive

[sbdchd]

TL;DR: suggestion to update no-throw to ban usage of Promise.reject()

Essentially this would make promises behave more like non-async code.

Currently with no-throw, throw is banned, so the following errors

function foo(): number {
  if (Math.random() > 0.5) {
    throw new Error("bad") // errors
  }
  return 1
}

and if we use async await we also get an error

async function bar(): Promise<number> {
  if (Math.random() > 0.5) {
    throw new Error("foo") // error
  }
  return 1
}

but we don't get an error if we explicitly use Promise.reject()

async function buzz(): Promise<number> {
  if (Math.random() > 0.5) {
    return Promise.reject(new Error("bad")) // no error
  }
  return 1
}

This is a problem as it represents the same hidden error state as the previous examples, which the rule errors on.

A more functional approach would be to instead return a union or result.

async function fizz(): Promise<number | Error> {
  if (Math.random() > 0.5) {
    return new Error("bad")
  }
  return 1
}

Then when used, both failure and success are warned about

fizz().then(r => {
  // now we have to handle both possible cases
  if (r instanceof Error) {
    console.error("error:", r)
  } else {
    console.log(r)
  }
})

[jonaskello]

This seems like a nice addition, either a separate rule or an extension to no-throw.

[jonaskello]

Fixed in #116

[RebeccaStevens]

Sorry I'm a bit late to this.
I understand the reasoning for this but I feel it is a bit of an anti-pattern for Promises.

In the example given, the return type is marked as Promise<number>; this could be though of as being equivalent to something like { value: number } | { error: Error }. The error state is not hidden here.
When the function returns, it would essential invoke a callback using logic something along the lines of the following.

if (result.error !== undefined) {
  handler.catch(result.error);
} else {
  handler.then(result.value);
}

Thus promises do not have a hidden error state using Promise.reject(), the code simply forks (which can still be considered functional).

buzz()
  .then(r => {
    console.log(r);
  })
  .catch(err => {
    console.error("error:", err);
  });

The issue with using Promise<number | Error> as a return type is that it basically undoes everything Promise were designed for and sends you back into callback land (aka callback hell).

Hidden error state can exist in Promise but this happens with async and await, not with Promise themselves.
For example:

async function foo(): Promise<number> {
  const x = await buzz(); // If buzz rejects, foo will reject too - i.e hidden error state exist here.
  return x + 2;
}

async function buzz(): Promise<number> {
  if (Math.random() > 0.5) {
    return Promise.reject(new Error("bad"));
  }
  return 1;
}

Hidden error state can be remove from this in several ways, here is one:

async function foo(): Promise<number> {
  const x = await buzz().catch(err => { // Hidden error state is explicitly caught making it no longer hidden.
    console.error("error:", err);
    return err;
  });

  return (
    x instanceof Error
      ? Promise.reject(x)
      : x + 2
  );
}

async function buzz(): Promise<number> {
  if (Math.random() > 0.5) {
    return Promise.reject(new Error("bad"));
  }
  return 1;
}

Creating a linting rule to catch hidden error state in Promises does not seem at all like an easy task.

I feel this addition to the rule should be reverted. Or possibly just made a option users can manually enable.

[jonaskello]

I think maybe it should be a separate rule. The name no-throw implies it applies to throw only. So maybe we could move it to a new rule called no-reject, no-promise-reject or something like that.

[jonaskello]

@sbdchd @RebeccaStevens Could you please check if #118 seems OK to solve this?

[sbdchd]

LGTM

[RebeccaStevens]

Seems good to me. I haven't tested it but I assume you have 😜

[jonaskello]

There was already a test added by @sbdchd so I just moved it :-).

The new no-reject rule is now released in 5.3.0 and the no-throw rule is reverted to the old behaviour. The previous version should probably have been a major release since it was not strictly additive (it added more checks to no-throw but also it altered its default behaviour). And following that, a new major version to change it back (altering the default behaviour back). However, since we are back to old behaviour I did not bother to do a major release for the latest change either.