Abazhenov / express-async-handler

Async Error Handling Middleware for Express
572 stars 39 forks source link

Correctly handle rejected promise whose value is a falsy value #25

Closed ehmicky closed 5 years ago

ehmicky commented 5 years ago

When the rejected promise is a falsy value (such as undefined or an empty string), express-async-handler calls next(falsyValue), Express interprets this as "the middleware function did not fail".

const expressAsyncHandler = require('express-async-handler');

const handler = async function() {
  // Same as throw Promise.reject(undefined)
  throw Promise.reject();
};

module.exports = expressAsyncHandler(handler)

One possible way to handle this would be:

const asyncUtil = fn =>
function asyncUtilWrap(...args) {
  const fnReturn = fn(...args)
  const next = args[args.length-1]
  return Promise.resolve(fnReturn).catch(error => 
    next(error instanceof Error ? error : new Error(String(error))))
}

module.exports = asyncUtil
MunifTanjim commented 5 years ago

Instead of adding that into the library (next(error instanceof Error ? error : new Error(String(error))))), the proper way would be catch the error yourself and re-throw it with your desired form:

const handler = expressAsyncHandler(async function() {
  try {
    // Same as throw Promise.reject(undefined)
    throw Promise.reject();
  } catch (err) {
    if (error instanceof Error) {
      throw error
    } else {
      throw new Error(String(error))
    }
  }
})

Because having an async function rejected with falsy value is not that common and it's a special case. So, it should be handled outside the library.

ehmicky commented 5 years ago

It is indeed improper to reject anything but an Error instance. It is not always possible to avoid though, since the improper promise rejection might come from a library (as opposed to the user code).

When it comes to adding a wrapper as @MunifTanjim suggests: the role of express-async-handler is to wrap express middleware with an async try-catch-like block. I believe requiring the users to add such block themselves might defeat the library purpose.

MunifTanjim commented 5 years ago

the role of express-async-handler is to wrap express middleware with an async try-catch-like block. I believe requiring the users to add such block themselves might defeat the library purpose.

The role of express-async-handle is actually calling the Express.js next function with the thrown value. And it does it.

If the next is called with falsy value, Express.js itself doesn't pass it to the error handling middleware. And it's the correct behavior. ~If you want that behavior changed you might want to open an issue on Express.js repository?~

Moreover, adding this behavior in express-async-handler would be encouraging a bad practice.

It is not always possible to avoid though, since the improper promise rejection might come from a library

If any library follows this practice then that library should be fixed instead.

But that's just my opinion. It's up to the maintainers what they want to do.