-
-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Comments
There is currently at least one open question: Should we allow |
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 |
I support this change. |
I kind of let this die. Let me me revisit the PR with the suggested changes and we can hopefully move forward! |
Just an update on this rule. The PR is all ready in |
I actually think this might bite a little bit, but, I still agree with it on principle. |
Ecosystem impact is moderate (4%).
It turns out that several of these were in code that wrote myself, so after fixing those up, the new impact is better:
A good 3-4 of them are actually strings being passed to a callback instead of an |
This will be part of the standard v10 beta. |
Fixes: standard/standard#623 There must be `undefined`, `null` or an error object in the first position of a callback.
@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. :) |
What about return cb(...someArray); why it'is error? |
@dutchenkoOleg, probably because you must pass |
The lack of |
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? |
FWIW the |
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 |
👍
On May 10, 2018 12:24 AM, "Feross Aboukhadijeh" <[email protected]> wrote:
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
<https://github.com/standard/eslint-plugin-standard/issues/27>
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#623 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAPBf3RTH-ttFxG_JhdqUMOyl7g16RJtks5tw-szgaJpZM4J8-hb>
.
|
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
Not allowed
Of course we can figure out if we should include
cb
andnext
as well ascallback
in the list of allowed function names.The text was updated successfully, but these errors were encountered: