-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
New: add no-async-promise-executor rule (fixes #10217) #10661
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments.
Wondering if the rule should allow non-global Promise
references?
|
||
## Rule Details | ||
|
||
This rule aims to... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs more 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I was aiming for concise documentation but I think I overdid it.
meta: { | ||
docs: { | ||
description: "disallow using an async function as a Promise executor", | ||
category: "Fill me in", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be Possible Errors
return { | ||
"NewExpression[callee.name='Promise'][arguments.0.async=true]"(node) { | ||
context.report({ | ||
node: context.getSourceCode().getFirstToken(node.arguments[0]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I imagine this wouldn't point at the right token if the node is surrounded by parens?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it still would be. I'll add a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I'm wondering if we should just use context.getSourceCode().getFirstToken(node.arguments[0], { filter: t => t.value === "async" })
or similar to be absolutely safe.
I think it's more common for people to do things like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
What is the purpose of this pull request? (put an "X" next to item)
[x] New rule (#10217)
What changes did you make? (Give an overview)
This implements the
no-async-promise-executor
rule, as described in #10217.Is there anything you'd like reviewers to focus on?
Nothing in particular