New Rule: no-callback-literal (fixes #12)

[xjamundx]

Tries to more strictly enforce the callback pattern. This will help people avoid the common mistake of calling back with an error message instead of an error object, which can lead to all sorts of weird bugs.

There is already this similar rule in ESLint:
http://eslint.org/docs/rules/no-throw-literal

There are probably some async patterns out there that this will wreck, so we need to test it with more than just my code base :)

[xjamundx]

@feross I'll mess around with npm link and see if I can hook it into the standard testsuite and see how many issues this causes.

[feross]

missing a 2

[xjamundx]

oh yeah, we should progbably use the fancy new "error" syntax here, but yes let me fix!

[feross]

Nice -- I'm running the test suite with this enable right now. Will report the output in a few minutes.

[xjamundx]

Here's what I get when i run the tests:

# tests 422
# pass  408
# fail  14

Let me highlight some of the examples:

// /Users/jamuferguson/dev/standard/tmp/webtorrent-desktop/src/main/external-player.js
function checkInstall (playerPath, cb) {
  // check for VLC if external player has not been specified by the user
  // otherwise assume the player is installed
  if (playerPath == null) return vlcCommand((err) => cb(!err))
  process.nextTick(() => cb(true))
}

//  /Users/jamuferguson/dev/standard/tmp/bonjour/lib/registry.js
  function done (exists) {
    mdns.removeListener('response', onresponse)
    clearTimeout(timer)
    cb(!!exists)
  }

// /Users/jamuferguson/dev/standard/tmp/yargs/test/completion.js
yargs(['--get-yargs-completions'])
   .completion('completion', function (current, argv, cb) {
      setTimeout(function () {
        cb(['apple', 'banana'])
       }, 10)
   })

// /Users/jamuferguson/dev/standard/tmp/socketeer/src/SessionManager.js
next({
    isOkay: true,
    token: token
})

// /Users/jamuferguson/dev/standard/tmp/couchdb-push/index.js
  try {
    db = nanoOption(db)
  } catch (e) {
    return callback({ error: 'invalid_db', reason: 'Not a valid database: ' + db })
  }

[feross]

Yeah, the webtorrent-desktop should be changed. That's confusing. The yargs one is in a test and could also easily be rewritten.

Any legit cases that we're failing on?

[xjamundx]

I added a few more examples to comment above. I don't know the use cases very well. Certainly, in my experience always calling back with an error object has proven to be a really good way to do it. Seems like in very specific cases people kind of throw together a custom error format (sometimes as simple as true|false), so I'd guess it'd probably require a bit more community input before deciding how to move forward.

[feross]

Here's an interesting case:

...
    .on('end', function () { cb(false, lines) })

Should false be allowed?

Other than this one case, I think most of the errors this caused could be probably rewritten to be clearer.

[xjamundx]

Any reason why false would be better than null or undefined?

[feross]

The true|false case is more about callback functions like fs.exists in node.js core that don't have an error at all, but just asynchronously return a boolean. This is now deprecated for a couple reasons, one of which is that it was the only callback function in node that wasn't calling back with an error in the first position.

Any reason why false would be better than null or undefined?

It's not better, but it's used in a few places in the clone test suite. So the question is whether to allow it or not.

[xjamundx]

If you allow false you may also need to allow !!var and similar.

[feross]

If you allow false you may also need to allow !!var and similar.

Not necessarily. false is very explicit, like null and undefined. !!var will always become either false or true and we never want to allow true.

[xjamundx]

Yeah I'm fine adding false as an allowed option

[feross]

I was just exploring the idea. Maybe we should keep it just as null? Is undefined allowed right now?

[xjamundx]

undefined is allowed currently, which again isn't normal, but IMO is better :)

[feross]

Okay, I'm fine with false then. The main point of this rule is to catch strings used in place of errors.

[xjamundx]

Updated the rule.

[dcousens]

ACK, but meh on false, it seems odd.

[feross]

I'm also okay with false being removed. Don't care much either way.

[feross]

Maybe the next step is to open an issue on the main standard repo and make the argument for this? Just so it has a bit more visibility and other collaborators have a chance to comment.

[xjamundx]

Yeah for sure, I'll do that. i think it's worth a discussino to figure this
out!

Thanks

On Wed, Sep 14, 2016 at 9:27 AM, Feross Aboukhadijeh <
notifications@github.com> wrote:

Maybe the next step is to open an issue on the main standard repo and
make the argument for this? Just so it has a bit more visibility and other
collaborators have a chance to comment.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#15 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAPBf1uhz0cfR7r7Zwa5ZcAyht-G3IRkks5qqCBtgaJpZM4J75rn
.

[xjamundx]

Okay I fixed the typo in the docs. I think this should be okay to merge if it has enough 👍

[feross]

Nice! I think we ought to keep this PR open until we're preparing to release standard v9

[LinusU]

I would also prefer not allowing false, great work!

[feross]

This will be part of the standard v10 beta.

Sorry that this didn't make it into v9. (The release just got too big so I had to stop merging rules!)

[feross]

Just sent a PR to remove the exception for false: #20

[feross]

Take it back. This disrupts some repos and I'm already including another big change in v10, so let's hold off on this til v11. But I want to do fewer, quicker, releases, so I don't think that it will be too much longer.

[xjamundx]

👍🏼

…

On Wed, Mar 1, 2017 at 10:18 PM Feross Aboukhadijeh < ***@***.***> wrote: Take it back. This disrupts some repos and I'm already including another big change in v10, so let's hold off on this til v11. But I want to do fewer, quicker, releases, so I don't think that it will be too much longer. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#15 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPBf2Ho_m4yPzwcNZHawA4uvW8osXjgks5rhl9QgaJpZM4J75rn> .

[feross]

I take back the take-back. Let's ship it in v10. Impact is less than I originally thought. See: standard/standard#623 (comment)

[feross]

Released as 2.1.0