Should a Promise.Reject Message Be Wrapped in Error

Should a Promise.reject message be wrapped in Error?

Yes, it most definitely should. A string is not an error, when you have errors usually it means something went wrong which means you'd really enjoy a good stack trace. No error - no stack trace.

Just like with try/catch, if you add .catch to a thrown rejection, you want to be able to log the stack trace, throwing strings ruins that for you.

I'm on mobile so this answer is rather short but I really can't emphasize enough how important this is. In large (10K+ LoC) apps stack traces in rejections really made the difference between easy remote bug hunting and a long night in the office.

Should I throw an error or return a rejected promise inside an async function?

They are correct.

The call to myCustomFunction assumes that a promise is returned at all times (.then and .catch deal with resolved and rejected promises, respectively). When you throw an error, the function doesn't return a promise.

You could use this to catch the error:

try {
  myModule.myCustomFunction(someInput).then(result => {
    // carry on
  })
  .catch(err => {
    // do something with the error
  })
} catch(err) {
  ...
}

But as you can see, this results in two error handlers: try/catch for the synchronously thrown error, and .catch for any rejected promises that sns.createTopic(someParams) may return.

That's why it's better to use Promise.reject():

module.exports.myCustomFunction = input => {

    if (badInput) {
        return Promise.reject('failed');
    }

    return sns.createTopic(someParams).promise()
}

Then, the .catch will catch both types of errors/rejections.

NB: for newer versions of Node.js (v7.6 and up, I believe), the following will also work:

module.exports.myCustomFunction = async input => {

    if (badInput) {
        throw new Error('failed');
    }

    return sns.createTopic(someParams).promise()
}

The key here is the async keyword. By using this keyword, the function results are wrapped by a promise automatically (similar to what @peteb's answer is showing).

Should Promise be rejected with Error or string?

A look at Mozilla's documentation is helpful: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/reject

Promise.reject("Testing static reject").then(function(reason) {
  // not called
}, function(reason) {
  console.log(reason); // "Testing static reject"
});

Promise.reject(new Error("fail")).then(function(error) {
  // not called
}, function(error) {
  console.log(error); // Stacktrace
});

It shows both strings and errors as valid "reasons" for a rejection. The main thing (I think) is that the "reason" should be meaningful.

If a stack trace is helpful then it may be better to provide an error. If a simple string is all that's needed, then it will suffice.

JavaScript Promises - reject vs. throw

There is no advantage of using one vs the other, but, there is a specific case where throw won't work. However, those cases can be fixed.

Any time you are inside of a promise callback, you can use throw. However, if you're in any other asynchronous callback, you must use reject.

For example, this won't trigger the catch:

new Promise(function() {
  setTimeout(function() {
    throw 'or nah';
    // return Promise.reject('or nah'); also won't work
  }, 1000);
}).catch(function(e) {
  console.log(e); // doesn't happen
});

await Promise.reject or throw error to bail out?

It's semantically correct to use throw in promise control flow, this is generally preferable way to bail out of promise chain.

Depending on coding style, await Promise.reject(...) may be used to differentiate between real errors and expected rejections. Rejected promise with string reason is valid but throw 'invalid result' is considered style problem that may be addressed with linter rules, because it's conventional to use Error instances as exceptions.

The reason why it's important is because string exceptions can't be detected with instanceof Error and don't have message property, consistent error logging as console.warn(error.message) will result in obscure undefined entries.

// ok
class Success extends Error {}
try {
  throw new Success('just a friendly notification');
} catch (err) {
  if (!(err instanceof Success)) {
    console.warn(err.message);
    throw err;
  }
}

// more or less
const SUCCESS = 'just a friendly notification';
try {
  await Promise.reject(SUCCESS);
} catch (err) {
  if (err !== SUCCESS)) {
    console.warn(err.message);
    throw err;
  }
}

// not ok
try {
  throw 'exception';
} catch (err) {
  if (typeof err === 'string') {
    console.warn(err);
  } else {
    console.warn(err.message);
  }

  throw err;
}

Since invalid result is actually an error, it's reasonable to make it one:

  throw new TypeError('invalid result');

I am not talking about the promise chain(the whole point of my question), so I don't think the thread JavaScript Promises - reject vs. throw answered my question.

async function is syntactic sugar for promise chain, so all points that are applicable to promises are applicable to async as well.

There may be cases when throwing an error is not the same as rejecting a promise, but they are specific to other promise implementations like AngularJS $q and don't affect ES6 promises. Synchronous error in Promise constructor results in exception, this also isn't applicable to async.

Nodejs Promise reject wont forward error messages

You have a slight misunderstanding of what's going on here. Calling reject will reject the promise, and the subsequent resolve will not resolve it. So far so good. The problem is that it won't stop execution without a return. So you're essentially running unnecessary code which happens to cause an error.

You should ensure that the resolve code is not executed in the case of an error. You can accomplish this with return reject or by wrapping the resolve in an else block.

Keep in mind there's nothing special about resolve/reject here. They're simple function calls and function calls do nothing to stop execution.

Promise reject() causes Uncaught (in promise) warning

This happens because you do not attach a catch handler to the promise returned by the first then method, which therefore is without handler for when the promise rejects. You do have one for the promise p in the last line, but not for the chained promise, returned by the then method, in the line before it.

As you correctly added in comments below, when a catch handler is not provided (or it's not a function), the default one will throw the error. Within a promise chain this error can be caught down the line with a catch method callback, but if none is there, the JavaScript engine will deal with the error like with any other uncaught error, and apply the default handler in such circumstances, which results in the output you see in the console.

To avoid this, chain the .catch method to the promise returned by the first then, like this:

p.then( result =>  console.log('Fulfilled'))
 .catch( error =>  console.log(error) );

What happens if you don't resolve or reject a promise?

A promise is just an object with properties in Javascript. There's no magic to it. So failing to resolve or reject a promise just fails to ever change the state from "pending" to anything else. This doesn't cause any fundamental problem in Javascript because a promise is just a regular Javascript object. The promise will still get garbage collected (even if still pending) if no code keeps a reference to the promise.

The real consequence here is what does that mean to the consumer of the promise if its state is never changed? Any .then() or .catch() listeners for resolve or reject transitions will never get called. Most code that uses promises expects them to resolve or reject at some point in the future (that's why promises are used in the first place). If they don't, then that code generally never gets to finish its work.

It's possible that you could have some other code that finishes the work for that task and the promise is just abandoned without ever doing its thing. There's no internal problem in Javascript if you do it that way, but it is not how promises were designed to work and is generally not how the consumer of promises expect them to work.

As you can see if the status is 401, I am redirecting to login page.
Promise is neither resolved nor rejected.

Is this code OK? Or is there any better way to accomplish this.

In this particular case, it's all OK and a redirect is a somewhat special and unique case. A redirect to a new browser page will completely clear the current page state (including all Javascript state) so it's perfectly fine to take a shortcut with the redirect and just leave other things unresolved. The system will completely reinitialize your Javascript state when the new page starts to load so any promises that were still pending will get cleaned up.

---

Related Topics