[no-promise-executor-return]: false positive on arrow function

[ghost]

What rule do you want to change?

no-promise-executor-return

Does this change cause the rule to produce more or fewer warnings?

Fewer warnings

How will the change be implemented? (New option, new default behavior, etc.)?

New default behaviour that could be changed via an option

Please provide some example code that this change will affect:

It is a common practice to have arrow functions as executors in Promise. Those cases are written that way because they can be one-liners which is to improve the readability.

E.g. writing a delay function as this:

new Promise((resolve) => setTimeout(resolve, millis));

What does the rule currently do for this code?

It reports Return values from promise executor functions cannot be read

What will the rule do after it's changed?

If it is an arrow function, I believe it makes sense to suppress the warning. Although, there is a possibility to have a code such as:

const someValue = new Promise((resolve) => {
  // code
  // code
  return someValue; // which is wrong, aggre
})

Are you willing to submit a pull request to implement this change?

Not sure how to do this. It is definitely a false positive, because my intention was not consuming the value at all, but to have a one-liner.

[btmills]

👋 hi @ghaiklor-wix! In the original discussion around adding this rule, we explicitly decided not to make an exception for arrow functions, and you can read more about the rationale there. An arrow function is also shown as an example of incorrect code in the rule docs. For those reasons, I'm 👎 to an option that would allow any expression-bodied arrow functions. In most cases, adding curly braces makes the intent more clear, e.g. new Promise((resolve) => { setTimeout(resolve, millis); });.

However, in the original issue, we also contemplated a future enhancement to allow returning function calls as a shortcut. That option would allow your setTimeout() as written without curly braces. Is that something you'd be interested in @ghaiklor-wix? I could be convinced to be in favor of adding such an option, though definitely not as the default behavior because it is contrary to the intent of the rule. @mdjermanovic since you originally suggested it, what are your current thoughts on an option to allow returning call expressions?

[ghost]

That option would allow your setTimeout() as written without curly braces. Is that something you'd be interested in @ghaiklor-wix?

Yeah, it looks like that's the case. I'll try to generalize the case...

When you have an arrow function without curly braces

const someOneLiner = new Promise(resolve => doSomethingElseNoMatterWhatAndNoMatterTheResult(resolve));

So, if you:

• have an arrow function without curly braces, hence implicit return
• you have a call to resolve (I believe we can check it statically, we can take first and second arguments to Promise as identifiers and check if they have been called at last)

then more likely you have a correctly written piece of code without errors

[mdjermanovic]

In most cases, adding curly braces makes the intent more clear, e.g. new Promise((resolve) => { setTimeout(resolve, millis); });.

However, in the original issue, we also contemplated a future enhancement to allow returning function calls as a shortcut. That option would allow your setTimeout() as written without curly braces. Is that something you'd be interested in @ghaiklor-wix? I could be convinced to be in favor of adding such an option, though definitely not as the default behavior because it is contrary to the intent of the rule. @mdjermanovic since you originally suggested it, what are your current thoughts on an option to allow returning call expressions?

I personally prefer just new Promise((resolve) => { setTimeout(resolve, millis); });, but since there was a discussion about allowing some return <expression> patterns, I suggested that we consider them later because it shouldn't be the default behavior anyway. My opinion about such options is neutral at the moment.

[mdjermanovic]

After some thinking about it, I'm not in favor of adding options to this rule and other rules that disallow returning values from specific functions. I think we should keep these rules simple and support only "common" explicit code with curly braces and no return statements other than return;. All other variations are stylistic.

Allowing any function calls could introduce many false negatives, while the additional check for the usage of resolve and reject arguments seems complex and difficult to document. I believe any design we could think of would have some (if not all) of the following downsides: false negatives, confusing behavior, complex implementation; while still covering only some but not all patterns.

I'd maybe only support an option for => void and return void since it's explicit, and it would be clear to explain the option.

[btmills]

Allowing any function calls could introduce many false negatives, while the additional check for the usage of resolve and reject arguments seems complex and difficult to document. I believe any design we could think of would have some (if not all) of the following downsides: false negatives, confusing behavior, complex implementation; while still covering only some but not all patterns.

You're right - such an option would significantly complicate the rule for only marginal stylistic benefit. I was neutral before, but now I'm negatively inclined like you. I'll leave the issue open for now to give anyone else on the team time to register a different opinion.

[ghost]

I was thinking after a while and experimenting with different styles and I agree that new Promise(resolve => { ... }) is good enough to have. So, maybe we need not do anything.

P.S. Although, it conflicts with another rule that checks for "only one statement per line".

[eslint-deprecated]

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

[mmkal]

@btmills I'd also like to allow expression arrow functions. In projects that use prettier, I've tended to see this rule just get turned off, because it expands

new Promise(resolve => { setTimeout(resolve, 1000) })

into

new Promise(resolve => {
  setTimeout(resolve, 1000)
})

And taking up three lines for such a simple thing just feels wrong. It's not the hugest deal, but it makes codebases more cluttered, so it gets turned off.

I'd like an option. But failing that, what if it's allowed when the return value is explicitly voided?

new Promise(resolve => void setTimeout(resolve, 1000))

Would the team be open to a PR doing one of these things?