-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
feat(eslint-plugin): [no-floating-promise] add option to ignore IIFEs #1799
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
Conversation
Thanks for the PR, @anikethsaha! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day. |
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.
could you please gate this functionality behind an option, as per my comment: #647 (comment)
I want end-users to opt-in to this functionality so that they're consciously making the decision for a little unsafety from the rule.
ok cool, also it should only check for async iife right ? not just iife ? |
it should check both - it should just be looking for an IIFE that returns a promise. |
make sense , ok creating an options name |
@bradzacher I think we need to change the selector for AST as well, cause currently in I have this check if (options.ignoreIife && isAsyncIife(node)) {
return;
} And, here is the definition of function isAsyncIife(node: TSESTree.ExpressionStatement): Boolean {
if(node?.expression.type === AST_NODE_TYPES.AwaitExpression){
return true
}
if (node?.expression.type === AST_NODE_TYPES.CallExpression) {
if (possibleIifeCalleType.has(node?.expression?.callee.type)) {
if (node?.expression?.callee?.hasAsync) {
return true ;
}
return node?.expression?.callee?.body?.body.some(hasPromise);
}
}
return false;
} And here is function hasPromise( node: TSESTree.ExpressionStatement | TSESTree.ReturnStatement): Boolean {
// has promise statement
if (node?.expression.type === "NewExpression" && node?.expression?.callee.name === "Promise") {
return true;
}
if (
// returning promise
node.type === AST_NODE_TYPES.ReturnStatement &&
node?.argument.type === AST_NODE_TYPES.NewExpression &&
node?.argument?.callee.name === "Promise"
) {
return true;
}
return false
} It looks like when I give input like this // options : [{ ignoreIife : true }]
(async function() {
// error cause of this
await res(1);
})(); still the there is a floating error. I did try changing the selectors ["ExpressionStatement > :not(ExpressionStatement )"](node){
check(node)
},
"ExpressionStatement > ExpressionStatement "(node){
if(node?.expression.type === AST_NODE_TYPES.AwaitExpression){
return true
}
check(node)
} Still no luck |
the code you've got currently in the PR doesn't look too far off. I'd expect something along the lines of: ExpressionStatement(node): void {
if (options.ignoreIIFE && isIIFE(node)) {
return;
}
// old code
}
function isIIFE(node: TSESTree.ExpressionStatement): boolean {
const expr = node.expression;
if (expr.type !== AST_NODE_TYPES.CallExpression) {
return false;
}
return (
expr.callee.type === AST_NODE_TYPES.ArrowFunctionExpression ||
expr.callee.type === AST_NODE_TYPES.FunctionExpression
);
} To clarify what this option should do: These cases should error with (function foo() { return Promise.resolve() })();
(async function foo() { return Promise.resolve() })();
(async () => { return Promise.resolve() })(); However, setting the option to true should not ignore the contents of an IIFE. I.e. this should still error with (async () => {
Promise.resolve(); // error - floating promise
})();
// no other lint error |
@bradzacher should I add custom type for |
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.
a few suggestions for you
Codecov Report
@@ Coverage Diff @@
## master #1799 +/- ##
=======================================
Coverage 94.37% 94.37%
=======================================
Files 164 164
Lines 7572 7579 +7
Branches 2175 2178 +3
=======================================
+ Hits 7146 7153 +7
Misses 183 183
Partials 243 243
|
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.
a few final comments, otherwise LGTM - thanks for this.
packages/eslint-plugin/tests/rules/no-floating-promises.test.ts
Outdated
Show resolved
Hide resolved
packages/eslint-plugin/tests/rules/no-floating-promises.test.ts
Outdated
Show resolved
Hide resolved
make sure you run prettier over your code |
Co-Authored-By: Brad Zacher <brad.zacher@gmail.com>
Co-Authored-By: Brad Zacher <brad.zacher@gmail.com>
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.
thanks!
fixes #647
added a no check for iife
isIife
(please do suggest if the logic there doesn't cover all cases)