67

I'm using promises and have code that looks like the following:

function getStuff() { 
  return fetchStuff().then(stuff => 
    process(stuff)
  ).catch(err => {
    console.error(err);
  });
}

Or:

async function getStuff() { 
  try {
    const stuff = await fetchStuff();
    return process(stuff);
  } catch (err) { 
    console.error(err);
  }
}

I was doing this to avoid missing on errors but was told by a fellow user that I shouldn't be doing this and it is frowned upon.

  • What's wrong with return ….catch(err => console.error(err))?
  • I've seen a lot of code that does this, why?
  • What should I do instead?
9
  • "catch-void antipattern" maybe?
    – Bergi
    Jun 17, 2018 at 12:39
  • 1
    @David784 I'm sorry for that! (that's my fault). The wording is unfortunate and I've been meaning to change it since we landed it in 7.7. Madara made the errors a lot nicer in v10 and we're going to fix that very soon - unhandled rejections will terminate the process if they get garbage collected by default in v11 and v12 and the deprecation warning will be gone. Jun 17, 2018 at 14:24
  • 1
    @David784 and you can track current progress in github.com/nodejs/promise-use-cases/issues/27 - I've edited my answer a bit to try and reflect that. Jun 17, 2018 at 14:25
  • 1
    Undercutting the question a bit, I'd be highly skeptical of someone who tells you not to do something, but then doesn't explain why that is the case. If this fellow user has a valid argument against using it, they should mention it. Or you should ask them for clarification first; since we can't know the exact contect (or their opinion).
    – Flater
    Jun 18, 2018 at 6:51
  • 1
    @Flater Note I wrote both the question and the answer, I did this since this sort of thing comes up a lot when we analyzed repos and code "in the wild" for the Node.js summit. In fact I did it exactly so that we can point people to "why". Jun 18, 2018 at 6:56

5 Answers 5

61

Why does old code do this?

Historically, older (pre 2013) promise libraries 'swallowed' unhandled promise rejections you have not handled yourself. This has not been the case in anything written since then.

What happens today?

Browsers and Node.js already automatically log uncaught promise rejections or have behaviour for handling them and will log them automatically.

Moreover - by adding the .catch you are signalling to the method calling the function that undefined is returned:

// undefined if there was an error
getStuff().then(stuff => console.log(stuff)); 

The question one should be asking oneself when writing async code is usually "what would the synchronous version of the code do?":

function calculate() { 
  try {
    const stuff = generateStuff();
    return process(stuff);
  } catch (err) { 
    console.error(err);
    // now it's clear that this function is 'swallowing' the error.
  }
}

I don't think a consumer would expect this function to return undefined if an error occurs.

So to sum things up - it is frowned upon because it surprises developers in the application flow and browsers log uncaught promise errors anyway today.

What to do instead:

Nothing. That's the beauty of it - if you wrote:

async function getStuff() { 
  const stuff = await fetchStuff();
  return process(stuff);
}
// or without async/await
const getStuff = fetchStuff().then(process);

In the first place you would get better errors all around anyway :)

What to do if I'm running an old version of Node.js?

Old versions of Node.js might not log errors or show a deprecation warning. In those versions you can use console.error (or proper logging instrumentation) globally:

// or throw to stop on errors
process.on('unhandledRejection', e => console.error(e));
16
  • 1
    When I log errors like this (assuming there is no problem with the return value being undefined or I'm re-throwing the error), I add information like the file name, line number, etc. or a simple message that gives context. If I simply rely on "unhandledRejection" events this information is not present and many times it means not knowing what operation actually failed (i.e. no context).
    – Itay
    Jun 17, 2018 at 12:29
  • 2
    @Itay thanks for the comment. Browsers and Node will log the error like an exception - including file number, file name, etc. Note that it is perfectly fine to rethrow (that is, } catch(e) { attachMetadata(e); throw e; }) in order to attach metadata rather than console.error. Does that address your issue? Jun 17, 2018 at 12:32
  • 1
    @Itay note that this changed in recent versions of browsers and Node.js, and as you know - we're (Node and V8) going to ship async stack traces for production for everyone. Jun 17, 2018 at 12:33
  • 3
    process.on('unhandledRejection', ...)...never even thought to look for the existence of this. Thank you!!!!
    – David784
    Jun 17, 2018 at 14:33
  • 1
    @BenjaminGruenbaum Seems like the main hurdle is just learning that it exists. If I hadn't seen your Q/A today, I would've just kept up with the .catches. (As you say in your video, importance of stackoverflow/documentation can't be overemphasized!) Maybe it would also be possible to add a "for more information" link to the console warning that node throws on an unhandled promise rejection? The more signposts pointing in the right direction, the better the chances that more devs will find it
    – David784
    Jun 17, 2018 at 15:17
14

What's wrong with return ….catch(err => console.error(err))?

It returns a promise that will fulfill with undefined after you handled the error.

On it's own, catching errors and logging them is fine at the end of a promise chain:

function main() {
    const element = document.getElementById("output");
    getStuff().then(result => {
        element.textContent = result;
    }, error => {
        element.textContent = "Sorry";
        element.classList.add("error");
        console.error(error);
    });
    element.textContent = "Fetching…";
}

However if getStuff() does catch the error itself to log it and do nothing else to handle it like providing a sensible fallback result, it leads to undefined showing up in the page instead of "Sorry".

I've seen a lot of code that does this, why?

Historically, people were afraid of promise errors being handled nowhere which lead to them disappearing altogether - being "swallowed" by the promise. So they added .catch(console.error) in every function to make sure that they'd notice errors in the console.

This is no longer necessary as all modern promise implementation can detect unhandled promises rejections and will fire warnings on the console.

Of course it's still necessary (or at least good practice, even if you don't expect anything to fail) to catch errors at the end of the promise chain (when you don't further return a promise).

What should I do instead?

In functions that return a promise to their caller, don't log errors and swallow them by doing that. Just return the promise so that the caller can catch the rejection and handle the error appropriately (by logging or anything).

This also simplifies code a great lot:

function getStuff() { 
  return fetchStuff().then(stuff => process(stuff));
}

async function getStuff() { 
  const stuff = await fetchStuff();
  return process(stuff);
}

If you insist on doing something with the rejection reason (logging, amending info), make sure to re-throw an error:

function getStuff() { 
  return fetchStuff().then(stuff =>
    process(stuff)
  ).catch(error => {
    stuffDetails.log(error);
    throw new Error("something happened, see detail log");
  });
}

async function getStuff() {
  try {
    const stuff = await fetchStuff();
    return process(stuff);
  } catch(error) {
    stuffDetails.log(error);
    throw new Error("something happened, see detail log");
  }
}

Same if you are handling some of the errors:

function getStuff() { 
  return fetchStuff().then(stuff =>
    process(stuff)
  ).catch(error => {
    if (expected(error))
      return defaultStuff;
    else
      throw error;
  });
}

async function getStuff() {
  try {
    const stuff = await fetchStuff();
    return process(stuff);
  } catch(error) {
    if (expected(error))
      return defaultStuff;
    else
      throw error;
  }
}
5
  • Is possible to use catch to restore promise from error state and i.e. return promise fulfilled by fallback value?
    – bigless
    Jun 17, 2018 at 13:14
  • @Bergi it is a good practice to catch your errors but we are never really sure when the promise chain is going to end. In the future, one can always extend on the current chain. Catching errors instead of just Promise rejections always seems like an over-kill to me. It has always led to weird flow in the application.
    – yeshashah
    Jun 17, 2018 at 13:23
  • @bigless yes, just do promise.catch(error => { if (expected(error)) return fallback; else throw error; })
    – Bergi
    Jun 17, 2018 at 13:37
  • 1
    @yeshashah Not sure what you mean by "one can always extend on the current chain". Have a look at the main example in my answer: that's definitely the end of the promise chain, main doesn't return anything.
    – Bergi
    Jun 17, 2018 at 13:38
  • @Bergi IMO, your comment here should be part of your answer, as the pattern is common and not mentioning it can imply that you shouldn't be catching the errors you're expecting/can handle/can recover from, while generating a reject via throw for those errors you don't actually handle.
    – Makyen
    Jun 17, 2018 at 15:51
4

The reason you should not catch errors unless absolutely required (which is never) is that

Apart from swallowing promise rejections, catch handler also swallows any JS errors that occurs in any successive code run by the corresponding success handler.

Implications

  1. Once an error is caught by a catch handler, it is considered as done and handled. All the successive promise subscribers in the promise chain will call their success handlers instead of failure or catch handlers. This leads to weird behaviours. This is never the intended code flow.

  2. If a function at lower level like a service method (getStuff) handles errors in catch, it breaks the principle of Separation of Concerns. A responsibility of service handler should be solely to fetch data. When that data call fails, the application who is calling that service handler should manage the error.

  3. The catching of error in some function being caught by another one, results in weird behaviours all around and makes it really hard to track root causes of bugs. To track such bugs we have to enable the Break on Caught Exceptions in the Chrome dev console which will break on every catch and could take hours at an end to debug.

It is always a good practice to handle promise rejections but We should always do that using failure handler over catch handler. A failure handler will only catch Promise rejections and lets the application break, if any JS error occurs, which is how it should be.

2
  • This is exactly like try/catch handlers :) Aug 1, 2018 at 16:27
  • What is a failure handler? I don't see it in mdn. Just then, catch, and finally
    – PHP Guru
    Nov 15, 2020 at 4:13
2

error is much too generic, it is a catch all but there are only so many things that the operation would fail with, error is everything errorSomethingSpecific gives granularity

0

The most general statement here, that applies in languages beyond javascript, is don't 'catch' an error unless you plan to 'handle' the error. Logging is not handling.

i.e. In general, the best (only?) reason for a catch is to handle/'deal with' the error in a constructive way that allows the code to carry on without any further problems. And again, a line of logging probably never achieves that...

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.