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
Closed

New Rule: no-callback-literal #623

xjamundx opened this issue Sep 14, 2016 · 17 comments

Comments

@xjamundx
Copy link

xjamundx 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
Copy link
Author

There is currently at least one open question:

Should we allow callback(false)?

@dougwilson
Copy link

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
Copy link
Member

feross commented Sep 14, 2016

I support this change.

@feross feross modified the milestone: standard v9 Sep 16, 2016
@xjamundx
Copy link
Author

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

@feross
Copy link
Member

feross 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
Copy link
Member

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
Copy link
Member

feross 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
Copy link
Member

feross commented Mar 2, 2017

This will be part of the standard v10 beta.

@feross feross closed this as completed Mar 2, 2017
feross added a commit to standard/eslint-config-standard that referenced this issue Mar 2, 2017
Fixes: standard/standard#623

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

@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. :)

@OlehDutchenko
Copy link

OlehDutchenko commented Jul 20, 2017

What about

return cb(...someArray);

why it'is error?

@tunnckoCore
Copy link

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

@dcousens
Copy link
Member

dcousens commented Dec 1, 2017

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

@timbl
Copy link

timbl 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
Copy link

@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
Copy link
Contributor

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

@feross
Copy link
Member

feross 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: https://github.com/standard/eslint-plugin-standard/issues/27

@xjamundx
Copy link
Author

xjamundx commented May 10, 2018 via email

@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.
Projects
Archived in project
Development

No branches or pull requests

8 participants