Tslint: Ability to check promises that miss error handling

Created on 19 Oct 2015  路  11Comments  路  Source: palantir/tslint

I work on a healthcare project where patient safety is extremely important and we really need to make sure that no errors are ignored. We use tslint extensively to improve code quality and consistency (thanks a lot, great tool).

It would be very helpful if there was a rule so that we could check that all promises have an error handler. Something like https://www.npmjs.com/package/thenlint

P2 Accepting PRs Rule Suggestion

Most helpful comment

@ajafff I believe no-floating-promises does not cover this issue. I think the intent of this issue (and the feature I came here looking for) is to cover the following:

async function doSomething(): Promise<void> {
  if (somethingIsWrong) {
     throw new Error("Something is wrong!");
  } 
}

// or, equivalently

async function doSomething(): Promise<void> {
  return new Promise<void>((resolve, reject) => {
    if (somethingIsWrong) {
      return reject("Something is wrong!");
    }
    return resolve();
  });
}

doSomething().then(() => doSomethingElse());

I would like a linter warning at the bottom line that says the call to doSomething needs to either set .catch, or be wrapped within a try/catch block. Otherwise, I'll get nasty "unhandled promise rejection" errors at runtime.

Also, it shouldn't matter to this proposed rule if the second doSomething is marked async or not. That is covered by promise-function-async.

All 11 comments

This is ultimately going to require #680 to be done 'properly' (i.e. catch all situations) I think

Well, I suspect it is complicated due to the chaining feature of promises, but for our application it would help even if all then()s are checked to have both their resolve and reject callbacks.

680 gives us types for everything, which actually makes it relatively straightforward from that point onwards....

A rough version using string matching could be done now... but probably not worth the effort.

+1, this is very needed for code consistency in promises chains.

How is this going?

Vote on this.Otherwise this is to simple to skip rejected promises. Right now chrome writes to console about unhandled errors in promises but only at runtime when resolve() or throw happens. It would be super cool to check in at build time

Just discovered this issue today while searching for a possible tslint solution to this same issue. We would also love for this to become a rule for all the reasons listed above.

That should be covered by no-floating-promises.

That should be covered by no-floating-promises.

@ajafff That was my initial understanding, too. But that rule does not cover this case:

async function isThisTrue(expression: string): Promise<boolean> {
    ....
}

if (isThisTrue()) {
    ...
}

Note how the if (isThisTrue()) is rather obviously missing the await.

Following my analysis in https://github.com/palantir/tslint/issues/3929, I have to conclude that no-floating-promises really only cares whether the Promise is assigned or used in any other way. The rule does not care that it is never handled.

Do we need another rule, say, no-unhandled-promises that verifies that Promises are either awaited, or alternatively handled via .catch()?

Do we need another rule, say, no-unhandled-promises that verifies that Promises are either awaited, or alternatively handled via .catch()?

Such a rule would be great @dscho! Does something like this exist already?
Actually I'm not really sure what the point of no-floating-promises is right now.

@ajafff I believe no-floating-promises does not cover this issue. I think the intent of this issue (and the feature I came here looking for) is to cover the following:

async function doSomething(): Promise<void> {
  if (somethingIsWrong) {
     throw new Error("Something is wrong!");
  } 
}

// or, equivalently

async function doSomething(): Promise<void> {
  return new Promise<void>((resolve, reject) => {
    if (somethingIsWrong) {
      return reject("Something is wrong!");
    }
    return resolve();
  });
}

doSomething().then(() => doSomethingElse());

I would like a linter warning at the bottom line that says the call to doSomething needs to either set .catch, or be wrapped within a try/catch block. Otherwise, I'll get nasty "unhandled promise rejection" errors at runtime.

Also, it shouldn't matter to this proposed rule if the second doSomething is marked async or not. That is covered by promise-function-async.

Was this page helpful?
0 / 5 - 0 ratings