Disallow returning value from Promise executor

[dwelle]

Please describe what the rule should do:

Disallows returning values of any kind from the Promise executor.

Value returned from Promise executor is ignored, but people may not be aware of it and they may think that returning e.g. another promise from the executor awaits that promise, or that the new promise (the outer one) is going to be rejected if the returned promise is rejected itself.

What category of rule is this? (place an "X" next to just one item)

[x] Warns about a potential error (problem)

Provide 2-3 code examples that this rule will warn about:

new Promise(() => {
    return Promise.resolve(42);
});

new Promise(() => {
    return false;
});

Why should this rule be included in ESLint (instead of a plugin)?

This rule would disallow returning anything from Promise executor function which is going to make original intent more clear, and prevent possible errors by expecting false behavior.

This rule is an extension of my previous (accepted) suggestion to help prevent issues when dealing with Promises.

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

Maybe.

[mdjermanovic]

Sounds reasonable to me 👍

[ljharb]

Ideally this would ignore return; or return resolve() or return reject() however, so that you can have patterns like:

new Promise((resolve, reject) => {
  if (condition) {
    return resolve();
  }
  reject();
});

new Promise((resolve, reject) => {
  if (condition) {
    resolve();
    return;
  }
  reject();
});

etc.

[dwelle]

@ljharb Yes. Come to think of it, there may be other valid cases along the same line which we would nonetheless not be able to support and people would need to refactor:

new Promise((resolve, reject) => {
    if (condition) {
        return getSomeData().then(() => resolve()).catch(() => reject());
    }
    resolve();
})

Thus, probably not make this rule the default (recommended)?

[mdjermanovic]

Removing the 'accepted' label as this doesn't have consensus yet (champion + 3 votes from other team members).

I can champion and work on this if no one has started yet, but it still needs one more 👍

[mdjermanovic]

I think that the rule should certainly ignore return;

As for return something just to make sure/emphasize that the function ends at that point, it seems that ignoring return resolve(); and return reject(); would just partially allow this pattern. Seems better to just have the "When Not To Use It" section in the documentation.

[mdjermanovic]

Should the rule also disallow implicit return from arrow functions:

new Promise((resolve, reject) => getSomething((err, data) => {
    if (err) {
        reject(err);
    } else {
        resolve(data);
    }
}));

Valid code would be:

new Promise((resolve, reject) => {
    getSomething((err, data) => {
        if (err) {
            reject(err);
        } else {
            resolve(data);
        }
    });
});

[dwelle]

@mdjermanovic I'd say, yes --- disallow. There's functionally no difference and this rule should prevent that to make intent clear. I understand for arrow functions it may be enticing to allow it, but it would defeat the purpose of this rule, IMO.

In fact, I'd not make any exceptions (except return;, of course) --- I personally changed my mind even on the return resolve()/return reject(), because even though it's innocuous (and will make writing certain cases easier), it again potentially introduces complexity. E.g.:

new Promise(resolve => {
  return resolve(someAsyncStuff())
});

Even though resolve doesn't return anything (and it wouldn't matter either way) it introduces tiny bit of complexity again because you need think for a bit on the significance of the return, before realizing there's none.

Not making exceptions also allows for 1) easier implem 2) not having to come up with smart explanations in docs for why the exception makes sense, and what downsides this exceptions introduces.

[mdjermanovic]

@dwelle I completely agree with everything from your comment.

not having to come up with smart explanations in docs for why the exception makes sense, and what downsides this exceptions introduces.

Also, an explanation could be confusing for a reader who doesn't use the pattern, even if it's in the 'When Not To Use It' section. Seems best to just have a return resolve() example in the incorrect examples.

I'm working on this. no-promise-executor-return sounds like a good name for this rule.

[silverwind]

Ideally this would ignore return; or return resolve() or return reject()

Seconding that. I'd suggest an option to further and allow returning any function call expressions. For example, a one-liner like return cleanUpSomething() is handy sometimes, even if it may be more "correct" to put it on two lines.

[dwelle]

@silverwind

return cleanUpSomething()

I don't see much reason why to complicate this rule. If you want to keep doing the above, then don't opt-in into this rule. Having exceptions would somewhat defeat the purpose of this rule, as stated in previous comments.

[silverwind]

Yeah, I would not enable that rule without such a option because it would bring a lot of false positives. I don't really care as I see a Promise constructor as a bad pattern in itself.

[mdjermanovic]

return cleanUpSomething() could be also a false negative?

I think that by default only return; should be allowed, but we can consider adding an option to allow return resolve()/return reject() (or any call expression) later as an enhancement.