False-positive for "error" causing issues in prefer-await-to-callbacks rule

[xjamundx]

see: #202 (comment)

The way that this rule detects a callback function is anything with a function signature of:

function(err) { // or now "error"
   // you look like a standard node.js callback function
}

The problem is plenty of people would do something like:

{errorsLists.map(error => <p>{error.message</[>})

Especially in JSX/react land and possibly elsewhere.

I could also see:

errors.forEach(error => console.log(error.message));

This rule is definitely not meant to apply in these scenarios, because these aren't actually the async style callbacks we're worried about, but....is there a better way to differentiate them?

I'd be pretty ok adding a JSX exception to this rule, or just like disable the rule if it doesn't apply to your app. Mostly I had node.js in mind when I made this :D

[xjamundx]

Both of my example show using standard array methods like map and forEach which are NEVER async, so I'd be super okay exempting them. The only exception is we need to make sure this is treated like a callback function:
https://caolan.github.io/async/v3/docs.html#map

But I thin kit's fine, because the callback signature is different (callback is 3rd argument)

[pkuczynski]

I have similar scenario with a false positive:

export const formatErrors = errors => errors.map((error) => { 
   ...
}

There is no promise involved in this piece of code, yet (error) => { is reported as breaking this rule...

[xjamundx]

Thanks, would love any kind of PR support on this if you have time @pkuczynski or maybe @iamdarshshah ?

[pkuczynski]

I agree with @jturel, that checking for a param name is a poor linting. However, I am flooded with work so I would not be able to come up with PR anytime soon :(

[iamdarshshah]

@xjamundx Sure, I can give it a try! I'll need your help alongside tho 😄

There is no promise involved in this piece of code, yet (error) => { is reported as breaking this rule...

@pkuczynski IMO - (Just keeping your use-case in mind) It would be great to first check whether the piece of code involves a Promise or not. And on the basis of that, it will report the error if that's the promise otherwise it won't.

Also, @xjamundx can you please provide the list of things that needs to be taken care of while thinking of any implementation?

This is what we need to implement (correct me if I'm not wrong):
We need to mutate a rule to prefer the use of async/await to the callback pattern if that callback involves a Promise or is an async callback.

I'd be pretty ok adding a JSX exception to this rule, or just like disable the rule if it doesn't apply to your app.

I agree with adding a JSX exception to this rule. But yes, we'll need to add a check for map and forEach or other sync methods.

[xjamundx]

We're proposing two exceptions to this rule:

1. If the direct parent is a common array function such as:

• .map
• .forEach
• .filter
• .every
• .some
• .find
• etc..

Ideally I'd like to exclude this library which has a similar, but not quite the same function signature as a lot of the normal array methods: https://caolan.github.io/async/v3/docs.html#map

so basically these should not warn:

getErrors().map(error => ....)
errors.filter(err => ....)
this.myErrors.forEach(function(error) { ... })

Also the lodash equivalents don't take callbacks

_.find(errors, function(err) { return  err.type === "CoolError" })

However these should complain

async.map(['file1','file2','file3'], fs.stat, function(err, results) {
     // this is a legit callback function
});
customMap(errors, (err) => {
   // ... there's no way i could know what was happening here
});

What about this ambiguous use case:

find(errors, function(err) { return  err.type === "CoolError" });

I'm going to say, we'll have a list of approved function/method names and if it's an exact match AND the callback is the only argument then it will not complain.

For now I think both of these will have to be errors, unless we can come up with a clever way to exclude them without also missing the async.map case.

import { map, find } from "lodash";
const myError = find(errors, function(err) { return  err.type === "CoolError" });
const messages = map(errors, function(error) { error.message });

We might be able to determine based on number of arguments, because the lodash versions would all have 2 arguments, but I think the async versions all have 3. Anyway, something to look into.

2. A wholesale JSX exception

Now what about JSX? I bet this doesn't come up very often outside of the map use case, so I'm willing to avoid a JSX-wide exception for the time being as long as we can get these exceptions in there.

Any other thoughts/questions?

[outoftime]

Another class of false positive: an action creator using redux-actions where the first parameter is an error:

export const gapiClientUnavailable = createAction(
  'GAPI_CLIENT_UNAVAILABLE',
  error => ({error}),
);

Since #202 is merely adding a new variant of the error param name, but does not introduce the overall strategy of using the param name as a heuristic for detecting callbacks, I’ll probably just disable the prefer-await-to-callbacks rule altogether; the approach seems too prone to false positives.

[xjamundx]

We just published 5.0 and I'd like to get this partial fix out there if we can soon. I may take a stab at it tonight.

Totally agree for many folks it may make sense to disable the rule as it's more relevant in node.js stacks.

[xjamundx]

I know this will come up again, so I will re-open so people can find this issue easily.

[xjamundx]

Published this with 5.1.0 but should probably also port to 4.x :-/