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

feat(eslint-plugin): add new rule no-floating-promises #495

Merged
merged 11 commits into from Jun 10, 2019

Conversation

princjef
Copy link
Contributor

@princjef princjef commented May 6, 2019

Adds the equivalent of TSLint's no-floating-promises rule. Fixes #464.

Some notes about the implementation:

  • The parent TSLint rule worked by identifying promises by type name (with options for additional type names). This solution instead identifies a value structurally as being promise-like, which is more robust and requires no configuration.
  • This rule does not make use of the isThenable function of tsutils to check whether a value should be handled, because thenable does not mandate the ability to reject/catch. Instead, a similar function is defined which is in line with the definition of a promise in A+ by requiring two parameters to exist in the .then() definition.
  • This rule mandates a parameter being passed to a .catch() call, unlike the previous rule. This is because a .catch() call with no function provided does not actually catch anything.
  • The determination of a value being promise-like will incorrectly return false for situations with .then() functions defined using rest params. For an initial implementation that has to check multiple positional parameters, this seemed like an substantial complication for a highly unlikely scenario, leading to at worst some errors being missed. The functionality can be added if desired.

@codecov
Copy link

codecov bot commented May 6, 2019

Codecov Report

Merging #495 into master will increase coverage by 0.07%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #495      +/-   ##
==========================================
+ Coverage   94.12%   94.19%   +0.07%     
==========================================
  Files         104      105       +1     
  Lines        4271     4323      +52     
  Branches     1166     1185      +19     
==========================================
+ Hits         4020     4072      +52     
  Misses        146      146              
  Partials      105      105
Impacted Files Coverage Δ
...es/eslint-plugin/src/rules/no-floating-promises.ts 100% <100%> (ø)
packages/eslint-plugin/src/rules/index.ts 100% <100%> (ø) ⬆️

@bradzacher bradzacher added the enhancement: new plugin rule New rule request for eslint-plugin label May 7, 2019
@bradzacher
Copy link
Member

Thanks for the contribution.
Looking at the examples, am I correct in saying that this code is considered invalid according to this rule:

const x = Promise.resolve()
x.then(() => {})
x.catch(() => {})

i.e. you have to then and catch within the same expression?

The docs LGTM, i haven't reviewed the rule code itself yet.

@princjef
Copy link
Contributor Author

princjef commented May 7, 2019

The code shown is invalid per the current implementation, but only on line 2. Specifically, the rule requires two handlers to be passed if the final call is a .then() call to ensure that failures are properly handled. Any of these forms will work:

const x = Promise.resolve();

x.then(() => {}, () => {}); // Valid because a rejection handler is provided
x.catch(() => {}); // Valid because rejections are handled
x.then(() => {}).catch(() => {}); // Valid because a catch appears at the end of the chain

And yes, if you .then() with no rejection handler and .catch() in a separate expression, it will fail. However, this should pass:

const x = Promise.resolve();
const y = x.then(() => {});
y.catch(() => {});

Adds the equivalent of TSLint's `no-floating-promises` rule. Fixes typescript-eslint#464
@princjef
Copy link
Contributor Author

princjef commented May 7, 2019

Realized that I made the check for what constituted a floating promise too aggressive. Softened it to be more conservative in raising errors and intelligently inspect more types of expressions.

@bradzacher
Copy link
Member

just a tip - you should avoid force pushing ammended commits after you've raised a PR.
if you do that we lose the history because github has no previous commits to track.
This makes it harder to review incrementally because I can't look at your changes since I last reviewed, so I have to re-review the entire PR.

We squash merge to master anyways, so it doesn't matter if there's junk commits in your branch.

@princjef
Copy link
Contributor Author

princjef commented May 8, 2019

Sorry about that. I'll be sure to break further updates into separate commits.

As a side note, have you considered adding a CONTRIBUTING.md file to the repository? I find that to be a useful place to codify practices like this in a discoverable way.

@bradzacher
Copy link
Member

Probably should now as we're getting more new contributors.

I never bothered because I was under the impression most people didn't read them, and I didn't have much contributing information that I thought really needed to be codified..

Though it's surprising for me that a large portion of our contributors use this force push workflow. I assume that it's because people are using a tool like git town?

@princjef
Copy link
Contributor Author

princjef commented May 8, 2019

I split time between GitHub and Azure DevOps depending on project and the latter doesn't have the same issue with diffing revisions on the same force pushed commits, making it less of a concern. I have also contributed to projects in the past that prefer to rebase merge and therefore like having the single commit despite the complications with reviewing.

For me personally, I just use plain git from the command line but I have aliases set up for commit --amend --no-edit and push --force-with-lease. I find it to be a faster workflow for developing (code review notwithstanding) because I don't have to craft the throwaway commit message.

As for contributing files, there will definitely be a large number of people who won't read it, but it gives an easy thing to refer people to when they do things wrong. You can even add a pull request template that has a checkbox for "following instructions in the CONTRIBUTING.md file" which will get more people to read it (as long as it's not too long).

bradzacher
bradzacher previously approved these changes May 8, 2019
@bradzacher bradzacher added the 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready label May 8, 2019
@jonathanrdelgado
Copy link
Contributor

First off, thank you for implementing this in ESLint. I don't intend this comment to detour from the work put in here. This is one of the rules I was really missing from ESLint's Typescript implementation.

I noticed this implementation specifically checks the sub-expression on void expressions. Some would find that useful, there is actually a good discussion here about this palantir/tslint#4653 -- however, I am of the camp where using void is actually useful when I want to explicitly allow a promise to be floating.

Would we consider perhaps adding configuration to this rule to support both cases? I have no problem putting in the corresponding work if that sounds reasonable.

@princjef
Copy link
Contributor Author

@jonathanrdelgado ah interesting, hadn't seen that usage pattern before but definitely not opposed to an option to allow it if it's a pattern people use. I would still be inclined to make the behavior as currently implemented the default so that people get the "safer" behavior unless they explicitly opt out.

With that said, I think the main question is whether the option is a one-off specifically for void expressions (e.g. allowVoid: true) or if it's something more generic (e.g. allowExpressions: ["VoidExpression"], akin to no-restricted-syntax).

The latter is more flexible, but has a couple of problems:

  1. The actual parsing of the inner expressions is done using the Typescript AST, but the option should probably be specified in terms of ESTree, so some translation would be needed
  2. There is no expression type for void expressions in ESTree (it's a UnaryExpression whose operator is void)

For these reasons, perhaps it's easier to start with allowVoid: true and then potentially add others later if needed. I don't know of any other expressions that are currently checked which someone would want to categorically allow.

@bradzacher
Copy link
Member

my opinion is that you shouldn't ever use the void operator in executed code.
IMO if that's something people do, then this rule should specifically check against it.
It's super simple to disable eslint rules with a comment, which makes things easy to codemod and grep for. Using the void operator to denote a non-floating promise is not easy to grep for, nor is it easy to codemod away.

@jonathanrdelgado
Copy link
Contributor

I think those are fair points and that seems reasonable to me. I'll move my existing void statements to disable comments. Thank you both for your input. Looking forward to this rule being included in future versions!

@princjef
Copy link
Contributor Author

Anything else I can do to help get this PR across the line?

@bradzacher
Copy link
Member

@princjef we like to have 2 approvals for feature PRs before merging.
Not all the maintainers have time week-to-week, so we leave the PRs open for a few weeks before merging to give everyone time to eyeball it.

That being said, this PR has been approved for a month, so it's about time for a merge.

@glen-84
Copy link
Contributor

glen-84 commented Jul 22, 2019

Regarding the use of void to suppress this rule, would an enhancement request to support this be closed without consideration?

IMO:

  • It's practically suggested by TypeScript developers (see here and here).

  • Other developers are using it for this purpose (see here).

  • It's a lot more terse. Compare:

    // eslint-disable-next-line @typescript-eslint/no-floating-promises
    example();

    with:

    void example();
  • It's easy to grep for, if it's only being used for this purpose.

@bradzacher
Copy link
Member

@glen-84 - please avoid commenting on old, merged PRs.
conversation on old PRs is not easily discoverable for people who want to ask similar questions, and it causes notification spam for the old PR creator / commenters.

Opening a new issue is much preferred.
Feel free to open a new issue and we can discuss there.

@typescript-eslint typescript-eslint locked as resolved and limited conversation to collaborators Jul 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready enhancement: new plugin rule New rule request for eslint-plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule proposal: no-floating-promises
4 participants