Consider removing no-callback-literal rule

[feross]

See discussion at: standard/eslint-plugin-standard#12 (comment)

[yibn2008]

I strongly recommend removing this rule. First of all, the use-cases of callback with literal is much more common than we expect, currently we can only workaround by change callback name. Secondly, as a non-configurable lint library, I think standard should be more careful for adding new rules, especially when the rules are not very common. Since we cannot configure it, we have to accept.

[feross]

Thanks for the feedback @yibn2008, considering it for the next version of standard

[LinusU]

I personally like this rule, the err-first callback-style is very widely used and I think it's nice to have to be explicit when not following it...

I don't have that strong of an opinion though...

[xjamundx]

I suggested this rule originally, but realize it has limited value and is susceptible to false positives.

[targumon]

I'm writing client side code.
Have functions for communicating with the server (async obviously).
Passing callbacks to them so once a response arrives, the code can do something with it (update the app state or whatever).

e.g.:

const askTheServerForSomething = (callback) => {
  post()
    .then(callback({x: 1})) // just an example. can be anything: object, string, boolean...
    .catch(ex => handle(ex))
}

standard complains about it.
I don't understand what programming pattern you're trying to make me avoid, but it feels really ridiculous that I overcome it simply by renaming callback to cbFunc.

[LinusU]

@targumon In your example, the problematic pattern is that you have no way of handling errors from the askTheServerForSomething method. If you want to use callbacks (instead of promises), the best practice way is to let the first argument to the callback be an optional error.

const askTheServerForSomething = (callback) => {
  post()
    .then(callback(null, {x: 1}))
    .catch(ex => callback(ex))
}

This ties into having self-contained functions that don't modify global state. In your example it seems like some global handler are invoked in the case of an error in the post() function. I would suggest instead putting that error handling in the outer part of the code:

askTheServerForSomething((err, res) => {
  if (err) return globalErrorHandler(err)

  // ...
})

It's a bit hard to make better recommendations based on the small example you provided, but I'd be happy to take on any follow up questions/discussion ☺️

[targumon]

@LinusU Yes, my example was only meant to illustrate the gist of the actual code.
In reality what my callback does is dispatching an action (which causes the app to move to a new state - similar to Redux) and inside catch I use it as well, just for dispatching a different action.

So it's not like I'm ignoring the errors. And as I said before the linter doesn't complain if I use a different name:

const doSomething = (cbFunc) => {
  post()
    .then(cbFunc({x: 1}))
}

This is a "cousin" of the handle-callback-err check which is only activated if the variable name is err or error:

const foo = (er) => { // this passes
  bar()
}

Anyway, thanks for your detailed reply - I learned from it! ^_^

[bookercodes]

I just encountered this error for the first time.

In the initial issue (issue #623), @xjamundx mentioned that passing a string "can lead to all sorts of weird bugs". What kind of weird bugs?

It's not a big deal of course, but I am curious: what is this rule protecting me from?

It's a tall order, and something I am sure has been suggested before, but a directory of rules/errors similar to https://eslint.org/docs/rules/ explaining each rules' behaviour and reasoning would be a delight ✨

Thank you for all your hard work on this library.

[xjamundx]

@bookercodes The simplest explanation looks something like this...

doSomething(function(err, data) {
    if (err) return console.warn(err.stack) // undefined
    console.log(data)
})

Or even this example, where did you have the error reading the file? Without the stack trace generated by new Error() you will never know.

readFile(filename, function(err, data) {
    if (err) console.log(err) // error reading file
})

My background as of late has been large enterprise apps where enforcing patterns like this helps save a ton of debugging time. We once had an issue like this where our uncaught exception handler was reporting undefined undefined for the error message for TWO MONTHS, which is how look it took for us to figure out that a third party lib was throwing strings, which are missing both the message and stack properties. That's what got me on the kick of linting away that pain.

Of course there are smaller apps and other patterns that wouldn't have these issues. There are also some systems which rely on passing other data types to a callback function done(false) in grunt for example, which is why I have a little bit of regret over pushing this rule onto standard ;-)

[bookercodes]

That is a very good point. In my case, I was dealing with an AWS Lambda function callback, where it is common to pass a string rather than an Error. Passing an Error works too so I'm going to do that just in case. If it doesn't hurt and migth help, I am down. Thanks for the response, I apprecaite it.

[LinusU]

@bookercodes we are running our entire infrastructure on Lambda and have this rule enabled, I would absolutely recommend passing errors instead of strings 👍

Passing the entire error will also give you the entire stack trace in the CloudWatch logs instead of just the error messages ☺️

[davestewart]

FYI, this rule doesn't play well with Electron when intercepting requests:

  session.defaultSession.webRequest.onBeforeRequest(filter, (details, callback) => {
    // some code
    callback({ cancel: false, ... })
  })

EDIT: Though reading @targumon's post above it's easy to solve by renaming to done().

[vokar]

So any plans to remove this rule? It's very inconvenient, and the way of detecting if a function is a callback by its name is rather hackish.
Especially annoying when adding standardjs to an old project.

[LinusU]

@vokar I still don't think that we should remove this rule. Would you mind sharing some examples of the code that you are having problem with? Knowing what the problem is will help us to decide if we want to remove this, or tweak it in some way...

[feross]

@bookercodes It's a tall order, and something I am sure has been suggested before, but a directory of rules/errors similar to https://eslint.org/docs/rules/ explaining each rules' behaviour and reasoning would be a delight ✨

We have that! See: https://standardjs.com/rules.html

[feross]

There are a few outstanding problems with this rule:

1. It can't handle the spread operator. Here's a minimal example:

module.exports = a

function a (cb) {
  cb(...[new Error(''), 1, 2]) // Unexpected literal in error position of callback.
}

I wonder if anyone has the interest to get it fixed up? Or perhaps we can see if the eslint-plugin-node folks want to adopt it.

2. It's the only rule from eslint-plugin-standard that we're currently using. See https://github.com/standard/eslint-config-standard/blob/05f6c64da7da67efe6638265e441cf43cb2d66fe/eslintrc.json#L209 If we remove it, we can get out of the eslint plugin maintenance business. It was something I was hoping to do (see Eliminate eslint-plugin-standard #1316) just to eliminate a dependency, but it's not super important to do so.

[mightyiam]

Back in July 2016, @feross wrote:

Yeah, I think this is a good idea. I like your list of callback, cb, and next. However, there's definitely a chance that there might be lots of false positives, but let's give it a try!

I would not mind if this rule is removed.

[feross]

I opened an issue asking if ESLint wants to adopt no-callback-literal so we can deprecate eslint-plugin-standard: mysticatea/eslint-plugin-node#178

If they adopt it, then we can just keep the rule enabled. It's been around for a long time and as folks continue to switch to async-await, I see this becoming more of a non-issue.

Although, maybe as folks migrate Node.js-style callbacks to async-await, more of the remaining callback cases will be false positives like what we've seen in this thread?

@LinusU do you have thoughts?

[LinusU]

If they adopt it, then we can just keep the rule enabled.

I think this is a good approach.

It's been around for a long time and as folks continue to switch to async-await, I see this becoming more of a non-issue.

Yeah

I've personally never been bit by a false positive here, but I have seen many example of bad code that this rule would have prevented.

All in all, I would prefer to have it enabled but don't feel super strongly about it ☺️

[feross]

eslint-plugin-node seems supportive of the idea, so I sent a PR: mysticatea/eslint-plugin-node#179

[feross]

Okay, let's keep the rule for now. Still open to potentially removing it in a few years once async-await has near 100% usage over callbacks.

Feel free to continue discussion, but closing this issue for now.

[ExtAnimal]

Does this rule exclude myObj.callback('opaqueParameter') ?

My callback method legitimately accepts a string.

[feross]

myObj.callback('opaqueParameter') is allowed. You can test it for yourself here: https://standardjs.com/demo.html

[nikugogoi]

There are a few outstanding problems with this rule:

1. It can't handle the spread operator. Here's a minimal example:

module.exports = a

function a (cb) {
  cb(...[new Error(''), 1, 2]) // Unexpected literal in error position of callback.
}

I wonder if anyone has the interest to get it fixed up? Or perhaps we can see if the eslint-plugin-node folks want to adopt it.

Hi, is this fixed? or is there a separate issue for this? @feross

[LinusU]

@nikugogoi doesn't seem like it's been fixed, and I cannot find an open issue. Would you be able to open an issue here?

https://github.com/mysticatea/eslint-plugin-node/issues

[alex-grover]

Another case where this rule returns a false positive:

I'm writing an async-validator rule for the browser for use with an antd form, and the validator interface expects an array of Error instances. However, the following causes a violation:

function validate(_, value, callback) {
  if (!value) return callback([new Error('Required')])
  callback()
}

As mentioned in some of the prior comments, it's possible to get around this rule by renaming the callback parameter to done or assigning the array to a variable rather than passing it directly. These don't seem like ideal solutions, and given the lack of configuration with this tool and the number of cases where callbacks don't fit the expectation I think it would make sense to remove it.