New rule: callback-return

[LinusU]

Suggesting that we include the rule callback-return

Currently all but one of our repos passes this test.

standard: Use JavaScript Standard Style (https://github.com/feross/standard) 
  standard/tmp/fastfall/bench.js:48:3: Expected return with your callback function. (callback-return)
  standard/tmp/fastfall/bench.js:49:3: Expected return with your callback function. (callback-return)
  standard/tmp/fastfall/bench.js:50:3: Expected return with your callback function. (callback-return)
  standard/tmp/fastfall/example.js:18:5: Expected indentation of 2 space characters but found 4. (indent)
  standard/tmp/fastfall/example.js:19:3: Expected indentation of 0 space characters but found 2. (indent)
not ok 88 fastfall

I have looked at the code and it's an easy fix, I could submit a pull request over there.

---

Enforce Return After Callback (callback-return)

The callback pattern is at the heart of most I/O and event-driven programming
in JavaScript.

function doSomething(err, callback) {
    if (err) {
        return callback(err);
    }
    callback();
}

To prevent calling the callback multiple times it is important to return anytime the callback is triggered outside
of the main function body. Neglecting this technique often leads to issues where you do something more than once.
For example, in the case of an HTTP request, you may try to send HTTP headers more than once leading node.js to throw
a Can't render headers after they are sent to the client. error.

Rule Details

This rule is aimed at ensuring that callbacks used outside of the main function block are always part-of or immediately
preceding a return statement. This rules decides what is a callback based on the name of the function being called.
By default the rule treats cb, callback, and next as callbacks.

The following patterns are considered problems:

/*eslint callback-return: 2*/

function foo() {
    if (err) {
        callback(err); /*error Expected return with your callback function.*/
    }
    callback();
}

The following patterns are not considered problems:

/*eslint callback-return: 2*/

function foo() {
    if (err) {
        return callback(err);
    }
    callback();
}

Options

The rule takes a single option, which is an array of possible callback names.

callback-return: [2, ["callback", "cb", "next"]]

Gotchas

There are several cases of bad behavior that this rule will not catch and even a few cases where
the rule will warn even though you are handling your callbacks correctly. Most of these issues arise
in areas where it is difficult to understand the meaning of the code through static analysis.

Passing the Callback by Reference

Here is a case where we pass the callback to the setTimeout function. Our rule does not detect this pattern, but
it is likely a mistake.

/*eslint callback-return: 2*/

function foo(callback) {
    if (err) {
        setTimeout(callback, 0); // this is bad, but WILL NOT warn
    }
    callback();
}

Triggering the Callback within a Nested Function

If you are calling the callback from within a nested function or an immediately invoked
function expression, we won't be able to detect that you're calling the callback and so
we won't warn.

/*eslint callback-return: 2*/

function foo(callback) {
    if (err) {
        process.nextTick(function() {
            return callback(); // this is bad, but WILL NOT warn
        });
    }
    callback();
}

If/Else Statements

Here is a case where you're doing the right thing in making sure to only callback() once, but because of the
difficulty in determining what you're doing, this rule does not allow for this pattern.

/*eslint callback-return: 2*/

function foo(callback) {
    if (err) {
        callback(err); // this is fine, but WILL warn /*error Expected return with your callback function.*/
    } else {
        callback();    // this is fine, but WILL warn /*error Expected return with your callback function.*/
    }
}

When Not To Use It

There are some cases where you might want to call a callback function more than once. In those cases this rule
may lead to incorrect behavior. In those cases you may want to reserve a special name for those callbacks and
not include that in the list of callbacks that trigger warnings.

Further Reading

• The Art Of Node: Callbacks
• Nodejitsu: What are the error conventions?

Related Rules

• handle-callback-err

---

I found the rule while comparing which rules xo has versus standard here: #216 (comment)

[mcollina]

I think this is a good rule 95% of the time, so 👍 for me. As for the remaining 5%, that can probably be configured with eslint comments. I remember than multiple return statements were bad for performance in V8, however I don't know if that still applies (for sure it'll cause a deopt).

[yoshuawuyts]

+1 here, seems like it catches errors before they get a chance to bite you.

[dcousens]

👍 absolutely ACK this.

This has caught me out so many times, I'd love it to be caught in standard.
As mentioned, the remaining 1% are exceptions and easily worked around.

[LinusU]

Cool 😎

I should mention that this is the setting that I tried: [1,["cb","callback","next","done"]]. Maybe it would be a good idea to add resolve and reject to the list as well thought. I'll suggest this to the xo project as well.

[dcousens]

[1, ["cb", "callback", "next", "done"]]

LGTM.

[LinusU]

Promises is another very common use case for this thought, which is why I suggested adding resolve and reject.

e.g.

function readFile (path) {
  return new Promise(function (resolve, reject) {
    fs.readFile(path, function (err, result) {
      if (err) reject(err) // <--- Notice missing return

      resolve(result)
    })
  })
}

[sindresorhus]

Be aware that this rule is pretty fragile atm. I had to disable it in XO because of too many false positives.

eslint/eslint#3420

[LinusU]

This could probably be revisited now that eslint/eslint#3559 is merged :)

[rstacruz]

[1, ["cb", "callback", "next", "done"]]

anyone else want to voice in on what other names can be used?

I personally use fn as a name in higher-order functions sometimes, typically with decorators, but i don't think that case is relevant to this rule.

[feross]

Just ran the tests with eslint 1.10.3 and this rule:

[2, ["cb", "callback", "next", "done"]]

All tests pass. This rule finally looks mature enough to use. Out of an abundance of caution, let's add it in v6.

[mcollina]

I agree.
Il giorno ven 25 dic 2015 alle 13:15 Feross Aboukhadijeh <
notifications@github.com> ha scritto:

Just ran the tests with eslint 1.10.3 and this rule:

[2, ["cb", "callback", "next", "done"]]

All tests pass. This rule finally looks mature enough to use. Out of an
abundance of caution, let's add it in v6.

—
Reply to this email directly or view it on GitHub
#298 (comment).

[bcomnes]

I like all the names discussed here.

[yoshuawuyts]

rad! v6 sounds good (':

[LinusU]

Could we add resolve and reject as well? I think it's a very good addition to people using promises.

[LinusU]

@feross Do you want a pull request for this? I would very much add resolve and reject as well.

[feross]

Actually, I take back my earlier comment about the tests passing. I must have ran the tests without the rule actually enabled.

Here's the latest results using the new thorough test suite for standard v6 that tests the 400 most popular packages on npm that use standard:

1..428
# tests 428
# pass  324
# fail  104

Full output: https://gist.github.com/feross/94b846d2150875bc7ecf

Unfortunately, I don't think this rule is reliable enough to enable by default for everyone. I like what it's trying to do, but we can't burden every standard user with understanding this rule's quirks.

When it works better, I'm looking forward to enabling it.