npm's guidelines on callback errors

问题:

I was reading through npm’s coding style guidelines and came across the following very cryptic suggestion:

Be very careful never to ever ever throw anything. It’s worse than useless. Just send the error message back as the first argument to the callback.

What exactly do they mean and how does one implement this behavior? Do they suggest calling the callback function within itself?

Here’s what I could think of using the async fs.readdir method.

fs.readdir('./', function callback(err, files) {
  if (err) {
    // throw err  // npm says DO NOT do this!
    callback(err) // Wouldn’t this cause an infinite loop?
  }
  else {
    // normal stuff
  }
})

回答1:

What they're trying to say is that you should design your modules so the asynchronous functions don't throw errors to catch, but are rather handled inside of a callback (like in the fs.readdir example you provided)...

So, for instance this is what they're saying you should design your module like:

var example = {
    logString: function(data, callback){
      var err = null;
      if (typeof data === "string") {
        console.log(data);
      } else {
        err = {"message": "Data is not a string!"};
      }
      callback(err);
    }
}

They want you to design it so the end user can handle the error inside of the callback instead of using a try/catch statement... For instance, when we use the example object:

example.logString(123, function(err){
  // Error is handled in callback instead of try/catch
  if (err) console.log(err)
});

This would log {"message": "Data is not a string!"}, because the data doesn't have a typeof equal to "string".

Here is an example of what they're saying you should avoid:

They don't want you to throw errors when you have your asynchronous callback at your disposal... So lets say we redesigned our module so the logString method throws an error instead of passing it into a callback... Like this:

var example = {
    logString: function(data, callback){
      if (typeof data === "string") {
        console.log(data);
      } else {
        // See, we're throwing it instead...
        throw {"message": "Data is not a string!"};
      }
      callback();
    }
}

With this, we have to do the whole try/catch statement, or else you'll get an uncaught error:

try {
  example.logString(321, function(){
    console.log("Done!")
  });
} catch (e) {
  console.log(e)
}

Final thoughts / Summary:

The reason I think NPM suggests this method is because it's simply more manageable inside of a asynchronous method.

NodeJS and JavaScript in general likes to have a asynchronous environment so nice to have it all compact into one place, error handling and all.

With the try/catch, it's just one more extra step you have to take, when it could EASILY be handled inside of the callback instead (if you're designing it asynchronously, which you should).

回答2:

Yes, that would cause an infinite loop. However, they're not talking about that type of callback. Instead, npm is referencing the callbacks used to interact with your module.

To expand upon your example:

module.exports = {
    getDirectoryFiles: function (directory, done) {
        fs.readdir(directory, function callback(err, files) {
            if (err) {
                return done(err);
            } else {
                return done(null, files);
            }
        })
    }
}

You should pass err to the callback from the scope above, not to the function with which you're currently dealing (in the above case, callback). The only reason to name those functions is to help with debugging.

The reason they say not to throw err is because node uses error-first callbacks. Everyone expects your library, if it uses callbacks, to propagate its errors as the first parameter to the callback. For example:

var yourLibrary = require("yourLibrary");

yourLibrary.getDirectoryFiles("./", function (err, files) {
    if (err) {
        console.log(err);
        // do something
    } else {
        // continue
    }
}