Why is `.catch(err => console.error(err))` discouraged?

Question

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?

Solution 1

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));

Solution 2

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;
  }
}

Solution 3

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.

Solution 4

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

View more solutions

Comments

[Itay]

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).

[Benjamin Gruenbaum]

@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?

[Benjamin Gruenbaum]

@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.

[Itay]

attachMetadata(e) seems like a valid solution, though I can't really know if I'm supposed to modify the thrown object. It seems like a better solution (if supported by the engine) would be as in Java - throwing another exception with the initial exception as the cause.

[Itay]

can you attach a link that describes async stack traces spec/proposal?

[bigless]

Is possible to use catch to restore promise from error state and i.e. return promise fulfilled by fallback value?

[yeshashah]

@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.

[Bergi]

@bigless yes, just do promise.catch(error => { if (expected(error)) return fallback; else throw error; })

[Bergi]

@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.

[David784]

process.on('unhandledRejection', ...)...never even thought to look for the existence of this. Thank you!!!!

[Benjamin Gruenbaum]

@David784 if there is any way you think we can make it clearer let me know. I share some of my pain when we were adding it in youtube.com/watch?v=LGpmUyFnyuQ - We've been thinking of writing a guide for using promises and async functions in Node.js (I originally wrote this for the guide but opted to post it here since there is no consensus on it yet).

[David784]

@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

[Benjamin Gruenbaum]

Thanks for the suggestion David, I will bring that up for discussion internally with the documentation team :) (Note that any error changes have to wait a major version because people who build tooling around them rely on the error format and we take backwards compatibility very seriously)

[danh]

Reading this thread, one could be forgiven the conclusion that app code shouldn't contain "catch" at all. Is that correct? In what circumstances is catch positively recommended?

[Makyen]

@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.

[Benjamin Gruenbaum]

@danh suggestions for improvement are very welcome! (thanks for bringing this up). .catch is not recommended unless you are actually handling the error. That is, if the .catch performed actual recovery for the error then it would have been appropriate. That is, you should let error propagate to the first method that can handle them and no further. Let me know if that answers your question and if so I'll edit it into the answer.

[danh]

Yes @BenjaminGruenbaum, thanks. Maybe to restate (and to check my understanding), lower on the stack, in order to cleanup and put the error into a form most consumable by the caller (e.g. controller code for a service, UI for an app, entry point for an api, etc)

[ComFreek]

I feel like .catching it in the main file, which bootstraps everything else, is not that bad (and acceptable until Node.js really terminates the process) and akin to try-catching in the main loop of C++ or Java code. What is your take on this?

[Benjamin Gruenbaum]

@ComFreek if you're handling everything then that's fine

[Ran Halprin]

@BenjaminGruenbaum - once we have async stack traces, we won't need to attach metadata, correct? Right now rejections are useless without it. Following the 'no catch' rule, we end up with general errors (e.g. "cannot find property .length of undefined") without real context (e.g. in a library that handles XML parsing), so we don't even know where to start looking

[Benjamin Gruenbaum]

@RanHalprin note we already have async stack traces with the inspector attached, the V8 team have agreed to work on this for production and Benedikt said it's a priority. Also, you always get the last frame - so it's "cannot read property .length of foo with a stack trace (not an async one though). We improved the warnings for Node 10 and Node 11 will improve the situation. You can track this in either github.com/nodejs/promise-use-cases or github.com/nodejs/promises-debugging

[Benjamin Gruenbaum]

This is exactly like try/catch handlers :)

[PHP Guru]

What is a failure handler? I don't see it in mdn. Just then, catch, and finally