Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New Rule: no-callback-literal #623

Closed
xjamundx opened this Issue Sep 14, 2016 · 17 comments

Comments

8 participants
@xjamundx
Copy link

commented Sep 14, 2016

What does everyone think of a rule that 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

Here is how it will work:

Allowed

callback(null)
callback()
callback(undefined)
callback(null, data)
callback(err)
callback(someRandomVariable)
callback(new Error("error!~"))

Not allowed

callback(true)
callback("there was an error")
callback(1)

Of course we can figure out if we should include cb and next as well as callback in the list of allowed function names.

@xjamundx

This comment has been minimized.

Copy link
Author

commented Sep 14, 2016

There is currently at least one open question:

Should we allow callback(false)?

@dougwilson

This comment has been minimized.

Copy link

commented Sep 14, 2016

Thankfully some time ago, Node.js added documentation around this: https://nodejs.org/dist/latest-v6.x/docs/api/errors.html#errors_node_js_style_callbacks Sadly reading through the whole page, the only things that are truly implicated is that it must be either null or an Error object, though I'm not sure how quickly that doc was written to take into account edge cases.

@feross

This comment has been minimized.

Copy link
Member

commented Sep 14, 2016

I support this change.

@feross feross added the enhancement label Sep 14, 2016

@feross feross modified the milestone: standard v9 Sep 16, 2016

@xjamundx

This comment has been minimized.

Copy link
Author

commented Nov 18, 2016

I kind of let this die. Let me me revisit the PR with the suggested changes and we can hopefully move forward!

@feross

This comment has been minimized.

Copy link
Member

commented Jan 14, 2017

Just an update on this rule. The PR is all ready in eslint-plugin-standard: standard/eslint-plugin-standard#15 We can merge when we're ready to do a standard v9 beta release.

@dcousens

This comment has been minimized.

Copy link
Member

commented Jan 14, 2017

I actually think this might bite a little bit, but, I still agree with it on principle.

@feross feross modified the milestones: standard v9, standard v10 Feb 9, 2017

@feross

This comment has been minimized.

Copy link
Member

commented Mar 2, 2017

Ecosystem impact is moderate (4%).

# tests 287
# pass  276
# fail  11

It turns out that several of these were in code that wrote myself, so after fixing those up, the new impact is better:

# tests 287
# pass  280
# fail  7

A good 3-4 of them are actually strings being passed to a callback instead of an Error object. A couple instances were cb(0) which is, I assume, a shorthand for cb(null). And the rest are legit uses, usually for a callback that returns a boolean for a use case like fs.exists, but those ought to be rewritten to use an error IMO. It's confusing for a callback to take a boolean as the first argument.

@feross

This comment has been minimized.

Copy link
Member

commented Mar 2, 2017

This will be part of the standard v10 beta.

@feross feross closed this Mar 2, 2017

feross added a commit to standard/eslint-config-standard that referenced this issue Mar 2, 2017

Enforce strict adherence to Node.js callback pattern
Fixes: standard/standard#623

There must be `undefined`, `null` or an error object in the first
position of a callback.
@tunnckoCore

This comment has been minimized.

Copy link

commented Mar 2, 2017

@feross I'm totally okey with that rule et al. But just tried the v10 beta, but it seems there something wrong in the impl. I'll post an issue to show for what i'm talking about.

Otherwise 🎉 for v10! Ten versions and still didn't have impact on my code :) hm, maybe once around v3-v4, don't remember. :)

@dutchenkoOleg

This comment has been minimized.

Copy link

commented Jul 20, 2017

What about

return cb(...someArray);

why it'is error?

@tunnckoCore

This comment has been minimized.

Copy link

commented Jul 20, 2017

@dutchenkoOleg, probably because you must pass err (or real error object) or null. I believe it is configured that way.

@dcousens

This comment has been minimized.

Copy link
Member

commented Dec 1, 2017

The lack of callback(...args) support is a bit lame.

@timbl

This comment has been minimized.

Copy link

commented Jan 6, 2018

Coming across this way after the fact, I think you really have decide whether your limits are. New this makes any API which does not start with an error param illegal Javascript. This was a big overreach IMMHO. We have APIs going back before node where the callback param is a success flag. Suddenly to be able to use everyone's favourite linter we have change our APIs?

@tunnckoCore

This comment has been minimized.

Copy link

commented Jan 6, 2018

@timbl 1. it's not about someone's linter, 2. such APIs are very old, at least. and if you didn't changed them for last 2 years, so..

@dcousens, no, it's not. It's rare case and also we are in very very good times of javascript on which you could not use callbacks et al.

@achingbrain

This comment has been minimized.

Copy link
Contributor

commented May 1, 2018

FWIW the pull-stream family of modules have you end streams early without erroring with cb(true) which this rule classes as invalid.

@feross

This comment has been minimized.

Copy link
Member

commented May 10, 2018

Hey everyone, just letting you all know that I'm considering removing this rule in standard v12.

You can follow this issue for updates: standard/eslint-plugin-standard#27

@xjamundx

This comment has been minimized.

Copy link
Author

commented May 10, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.