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

[Feature request] disallow async promise executor #10217

Closed
dwelle opened this issue Apr 14, 2018 · 6 comments
Closed

[Feature request] disallow async promise executor #10217

dwelle opened this issue Apr 14, 2018 · 6 comments
Assignees
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules

Comments

@dwelle
Copy link
Contributor

dwelle commented Apr 14, 2018

Please describe what the rule should do:

Promise executors must not be (technically, they can if you know what you're doing) async else bad side effects happen, such as throwing (seemingly) sync errors (A) no longer rejecting the promise (thus unable to be caught by (B)), because the async function is executed in different frame:

new Promise( async r => {
    throw new Error("aaa"); // (A)
})
.catch(() => {}) // (B)

Why is this different from not catching any potential async errors in a non-async executor? It isn't -- but, the above example gives off a feeling like you're throwing a sync error, while it's not, and for people new to async/await this may not be evident.

More info in this SO answer.

Thus, I propose a rule that forbids async executor function.

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

[ ] Enforces code style
[x] Warns about a potential error
[ ] Suggests an alternate way of doing something
[ ] Other (please specify:)

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

new Promise( async function (resolve, reject) {
    // any code
});
new Promise( async (resolve, reject) => {
    // any code
});
const executor = async function (resolve, reject) {
    // any code
};
new Promise( executor );

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

It seems like to become common-enough error when async/await becomes more mainstream to include it in core.

@eslint-deprecated eslint-deprecated bot added the triage An ESLint team member will look at this issue soon label Apr 14, 2018
@platinumazure platinumazure added the rule Relates to ESLint's core rules label Apr 14, 2018
@eslint-deprecated
Copy link

Hi @dwelle, thanks for the issue. It looks like there's not enough information for us to know how to help you.

If you're reporting a bug, please be sure to include:

  1. The version of ESLint you are using (run eslint -v)
  2. What you did (the source code and ESLint configuration)
  3. The actual ESLint output complete with numbers
  4. What you expected to happen instead

Requesting a new rule? Please see Proposing a New Rule for instructions.

Requesting a rule change? Please see Proposing a Rule Change for instructions.

If it's something else, please just provide as much additional information as possible. Thanks!

@platinumazure platinumazure added feature This change adds a new feature to ESLint evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion needs info Not enough information has been provided to triage this issue and removed triage An ESLint team member will look at this issue soon labels Apr 14, 2018
@platinumazure
Copy link
Member

Hi @dwelle, thanks for the issue. This sounds good to me and I would love to get behind it once we have a little more information.

Would you mind editing the initial issue post to reflect the new issue template, as requested by the bot? It would be good to see a few more examples. Thanks!

@dwelle
Copy link
Contributor Author

dwelle commented Apr 15, 2018

@platinumazure edited. I went through https://eslint.org/docs/developer-guide/contributing/new-rules before creating this issue, but didn't occur to me to click through the new issue link on the page as it wasn't clear enough it's a template. I issued a PR to make it clearer.

@platinumazure
Copy link
Member

platinumazure commented Apr 15, 2018 via email

@not-an-aardvark not-an-aardvark removed the needs info Not enough information has been provided to triage this issue label Jul 22, 2018
@not-an-aardvark not-an-aardvark self-assigned this Jul 22, 2018
@not-an-aardvark
Copy link
Member

I'll champion this change.

@eslint/eslint-team Anyone willing to support this change? It needs two more 👍s from team members to accept it.

@btmills
Copy link
Member

btmills commented Jul 23, 2018

Just added my 👍. This is now accepted!

@btmills btmills added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jul 23, 2018
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Jan 24, 2019
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jan 24, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion feature This change adds a new feature to ESLint rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests

4 participants