Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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 onhttps://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitorsper 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 |
anikethsaha commentedMar 26, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
@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 functionisAsyncIife(node:TSESTree.ExpressionStatement):Boolean{if(node?.expression.type===AST_NODE_TYPES.AwaitExpression){returntrue}if(node?.expression.type===AST_NODE_TYPES.CallExpression){if(possibleIifeCalleType.has(node?.expression?.callee.type)){if(node?.expression?.callee?.hasAsync){returntrue;}returnnode?.expression?.callee?.body?.body.some(hasPromise);}}returnfalse;} And here is functionhasPromise(node:TSESTree.ExpressionStatement|TSESTree.ReturnStatement):Boolean{// has promise statementif(node?.expression.type==="NewExpression"&&node?.expression?.callee.name==="Promise"){returntrue;}if(// returning promisenode.type===AST_NODE_TYPES.ReturnStatement&&node?.argument.type===AST_NODE_TYPES.NewExpression&&node?.argument?.callee.name==="Promise"){returntrue;}returnfalse} It looks like when I give input like this // options : [{ ignoreIife : true }](asyncfunction(){// error cause of thisawaitres(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){returntrue}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}functionisIIFE(node:TSESTree.ExpressionStatement):boolean{constexpr=node.expression;if(expr.type!==AST_NODE_TYPES.CallExpression){returnfalse;}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 (functionfoo(){returnPromise.resolve()})();(asyncfunctionfoo(){returnPromise.resolve()})();(async()=>{returnPromise.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
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
codecovbot commentedMar 29, 2020 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
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!
Uh oh!
There was an error while loading.Please reload this page.
fixes#647
added a no check for iife
isIife
(please do suggest if the logic there doesn't cover all cases)