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
Disallow returning value from Promise executor #12640
Comments
Sounds reasonable to me 👍 |
Ideally this would ignore new Promise((resolve, reject) => {
if (condition) {
return resolve();
}
reject();
}); new Promise((resolve, reject) => {
if (condition) {
resolve();
return;
}
reject();
}); etc. |
@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)? |
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 👍 |
I think that the rule should certainly ignore As for |
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);
}
});
}); |
@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 new Promise(resolve => {
return resolve(someAsyncStuff())
}); Even though 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. |
@dwelle I completely agree with everything from your comment.
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 I'm working on this. |
Seconding that. I'd suggest an option to further and allow returning any function call expressions. For example, a one-liner like |
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. |
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 |
I think that by default only |
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:
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.
The text was updated successfully, but these errors were encountered: