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

Prevent callback in try block #423

Closed
LinusU opened this issue Feb 17, 2016 · 3 comments

Comments

@LinusU
Copy link
Member

commented Feb 17, 2016

I misstake that I've seen quite a few people doing is calling a callback in a try block, not thinking about the fact that any error occurring in the callback will now be catched as well.

Example bad code:

try {
  cb(null, JSON.parse(data))
} catch (err) {
  cb(err)
}

What the authors intent really was:

let parsed
try {
  parsed = JSON.parse(data)
} catch (err) {
  return cb(err)
}

cb(null, parsed)

I understand that this might be a bit hard to implement, but I still think that it at the very least is worth discussing.

Would it be unreasonable to assume that every variable named cb is a callback? It seems to be the accepted practice to name it as such, in the same way that we have rules that assumes that variables named err are errors, and needs to be handled.

What do you think?

@dcousens

This comment has been minimized.

Copy link
Member

commented Feb 17, 2016

cb, next, done, callback, fin, finish, free, resolve?, reject? ... I feel like this could go on forever.
I agree conceptually though :)

@feross

This comment has been minimized.

Copy link
Member

commented Feb 19, 2016

Agree conceptually. The first step is to propose this in eslint. I'd prefer to not maintain our own rules, since that's a ton of work.

@feross feross changed the title Possible idea for rule Prevent callback in try block Feb 19, 2016

@feross feross added the blocked label Feb 19, 2016

@feross

This comment has been minimized.

Copy link
Member

commented Aug 19, 2016

I'm going to close this issue due to lack of progress.

This rule isn't really something that I, or any of the other standard collaborators that I know of, plan to implement at the moment. That's not to say it's a bad idea. I would accept a PR that enabled it, but the first step would be to get it added to ESLint or one of the ESLint plugins that we already depend on.

Then, send a PR to add the rule to standard, or open a new issue and I'm happy to do it.

@feross feross closed this Aug 19, 2016

@lock lock bot locked as resolved and limited conversation to collaborators May 10, 2018

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