Codecov Report

Merging#495 intomaster willincrease coverage by0.07%.
The diff coverage is100%.

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

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

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

i.e. you have tothen andcatch within the same expression?

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

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:

constx=Promise.resolve();x.then(()=>{},()=>{});// Valid because a rejection handler is providedx.catch(()=>{});// Valid because rejections are handledx.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:

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

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.

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.

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.

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?

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 forcommit --amend --no-edit andpush --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).

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 thispalantir/tslint#4653 -- however, I am of the camp where usingvoid 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.

@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 tono-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 aUnaryExpression whose operator isvoid)

For these reasons, perhaps it's easier to start withallowVoid: 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.

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.

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!

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

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

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

IMO:

• It's practicallysuggested by TypeScript developers (seehere andhere).

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

• It's a lot more terse. Compare:

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

  with:

  voidexample();

• It's easy to grep for,if it's only being used for this purpose.

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