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?

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

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

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

Answer 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