feat(eslint-plugin): [no-floating-promise] add option to ignore IIFEs #1799

anikethsaha · 2020-03-25T19:21:24Z

fixes #647
added a no check for iife

created a new function to detect iife isIife (please do suggest if the logic there doesn't cover all cases)
added tests

typescript-eslint · 2020-03-25T19:21:32Z

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.

bradzacher

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.

anikethsaha · 2020-03-25T19:27:21Z

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 ?

bradzacher · 2020-03-25T19:29:14Z

it should check both - it should just be looking for an IIFE that returns a promise.

anikethsaha · 2020-03-25T19:30:40Z

it should check both - it should just be looking for an IIFE that returns a promise.

make sense , ok creating an options name ignoreIife

anikethsaha · 2020-03-26T19:06:59Z

@bradzacher I think we need to change the selector for AST as well, cause

currently in ExpressionStatement

I have this check

if (options.ignoreIife && isAsyncIife(node)) {
  return;
}

And, here is the definition of isAsyncIife

    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 hasPromise

    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

bradzacher · 2020-03-26T19:21:01Z

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 ignoreIIFE: false (the default)
They should not error with ignoreIIFE: true

(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 ignoreIIFE: true

(async () => {
  Promise.resolve(); // error - floating promise
})();
// no other lint error

anikethsaha · 2020-03-27T08:09:37Z

@bradzacher should I add custom type for isAsyncIife method as I added TSESTree.ExpressionStatement | TSESTree.ArrowFunctionExpression | TSESTree.TSTypeAnnotation type to node but still typecheck is failing.
also, i tried node?.expression.typeAnnotation.typeand still it failed

bradzacher

a few suggestions for you

packages/eslint-plugin/docs/rules/no-floating-promises.md

packages/eslint-plugin/src/rules/no-floating-promises.ts

codecov · 2020-03-29T16:01:05Z

Codecov Report

Merging #1799 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           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

Flag	Coverage Δ
#unittest	`94.37% <100.00%> (+<0.01%)`	⬆️

Impacted Files	Coverage Δ
...es/eslint-plugin/src/rules/no-floating-promises.ts	`100.00% <100.00%> (ø)`

bradzacher

a few final comments, otherwise LGTM - thanks for this.

packages/eslint-plugin/docs/rules/no-floating-promises.md

packages/eslint-plugin/tests/rules/no-floating-promises.test.ts

packages/eslint-plugin/docs/rules/no-floating-promises.md

packages/eslint-plugin/tests/rules/no-floating-promises.test.ts

bradzacher · 2020-03-29T22:25:55Z

make sure you run prettier over your code

https://github.com/typescript-eslint/typescript-eslint/blob/master/CONTRIBUTING.md#validating-your-changes

Co-Authored-By: Brad Zacher <brad.zacher@gmail.com>

anikethsaha · 2020-04-01T13:37:30Z

same issue here as well

bradzacher

thanks!

bradzacher added the enhancement: plugin rule option New rule option for an existing eslint-plugin rule label Mar 25, 2020

bradzacher requested changes Mar 25, 2020

View reviewed changes

bradzacher added the awaiting response Issues waiting for a reply from the OP or another party label Mar 25, 2020

bradzacher changed the title ~~fix: ignores iife for no-floating-promise (fixes #647)~~ fix(eslint-plugin): [no-floating-promise] add option to ignore IIFEs Mar 25, 2020

anikethsaha force-pushed the fixes-647 branch from eb841a7 to 6e9e800 Compare March 26, 2020 21:29

bradzacher requested changes Mar 27, 2020

View reviewed changes

anikethsaha requested a review from bradzacher March 29, 2020 15:50

bradzacher requested changes Mar 29, 2020

View reviewed changes

packages/eslint-plugin/docs/rules/no-floating-promises.md Show resolved Hide resolved

packages/eslint-plugin/tests/rules/no-floating-promises.test.ts Outdated Show resolved Hide resolved

packages/eslint-plugin/docs/rules/no-floating-promises.md Outdated Show resolved Hide resolved

bradzacher reviewed Mar 29, 2020

View reviewed changes

packages/eslint-plugin/tests/rules/no-floating-promises.test.ts Outdated Show resolved Hide resolved

anikethsaha requested a review from bradzacher March 29, 2020 18:32

anikethsaha and others added 8 commits April 1, 2020 12:32

fix: ignores iife for no-floating-promise (fixes typescript-eslint#647)

ff3ce3d

feat: ignoreIIFE options and tests

f52b591

chore: docs and typing updates

7b754bb

chore: docs and types update

f705053

chore: formatting and docs refactore

03b18c7

Co-Authored-By: Brad Zacher <brad.zacher@gmail.com>

chore: docs formatting

6986405

Co-Authored-By: Brad Zacher <brad.zacher@gmail.com>

chore: test refactore and typecheck fix

325d9d8

docs: docs refactore

4b69717

anikethsaha force-pushed the fixes-647 branch from 2d4ace9 to 4b69717 Compare April 4, 2020 06:54

chore: linting fixes

2c8315f

anikethsaha added 2 commits April 4, 2020 07:14

chore: test fix

920ebdd

Merge branch 'master' into fixes-647

ed4f9b4

bradzacher removed the awaiting response Issues waiting for a reply from the OP or another party label Apr 4, 2020

Merge branch 'master' into fixes-647

836a7e8

bradzacher changed the title ~~fix(eslint-plugin): [no-floating-promise] add option to ignore IIFEs~~ feat(eslint-plugin): [no-floating-promise] add option to ignore IIFEs Apr 20, 2020

Update no-floating-promises.md

a26861b

bradzacher previously approved these changes Apr 20, 2020

View reviewed changes

Update no-floating-promises.md

6548b1c

bradzacher dismissed their stale review via 6548b1c April 20, 2020 05:12

Update no-floating-promises.md

2c2155e

bradzacher previously approved these changes Apr 20, 2020

View reviewed changes

bradzacher added 2 commits April 19, 2020 22:13

Merge branch 'master' into fixes-647

00d9312

Update no-floating-promises.md

b741c77

bradzacher dismissed their stale review via b741c77 April 20, 2020 05:20

Update no-floating-promises.md

05ed5f6

bradzacher merged commit cea51bf into typescript-eslint:master Apr 20, 2020

github-actions bot locked as resolved and limited conversation to collaborators May 21, 2020

Uh oh!

feat(eslint-plugin): [no-floating-promise] add option to ignore IIFEs #1799

feat(eslint-plugin): [no-floating-promise] add option to ignore IIFEs #1799

Uh oh!

Conversation

anikethsaha commented Mar 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-eslint bot commented Mar 25, 2020

Uh oh!

bradzacher left a comment

Choose a reason for hiding this comment

Uh oh!

anikethsaha commented Mar 25, 2020

Uh oh!

bradzacher commented Mar 25, 2020

Uh oh!

anikethsaha commented Mar 25, 2020

Uh oh!

anikethsaha commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bradzacher commented Mar 26, 2020

Uh oh!

anikethsaha commented Mar 27, 2020

Uh oh!

bradzacher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Mar 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bradzacher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bradzacher commented Mar 29, 2020

Uh oh!

anikethsaha commented Apr 1, 2020

Uh oh!

bradzacher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

anikethsaha commented Mar 25, 2020 •

edited

Loading

anikethsaha commented Mar 26, 2020 •

edited

Loading

codecov bot commented Mar 29, 2020 •

edited

Loading