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

Projects
None yet
3 participants
@princjef
Copy link
Contributor

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

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

commented May 7, 2019

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

This comment has been minimized.

Copy link
Contributor Author

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(() => {});
feat(eslint-plugin): add new rule no-floating-promises
Adds the equivalent of TSLint's `no-floating-promises` rule. Fixes #464

@princjef princjef force-pushed the princjef:no-floating-promises branch from f897a68 to 6bb84bd May 7, 2019

@princjef

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented May 7, 2019

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

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented May 8, 2019

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

This comment has been minimized.

Copy link
Contributor Author

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

@jonathanrdelgado

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

@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

This comment has been minimized.

Copy link
Member

commented May 17, 2019

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

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

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!

@bajtos bajtos referenced this pull request May 24, 2019

Merged

feat(build): add eslint scripts and default configs #2572

2 of 7 tasks complete

princjef added some commits Jun 9, 2019

@princjef

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

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

@bradzacher

This comment has been minimized.

Copy link
Member

commented Jun 10, 2019

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

@bradzacher bradzacher merged commit 61e6385 into typescript-eslint:master Jun 10, 2019

8 checks passed

Semantic Pull Request ready to be squashed
Details
codecov/patch 100% of diff hit (target 90%)
Details
codecov/project 94.19% (+0.07%) compared to 2c557f1
Details
typescript-eslint.typescript-eslint Build #20190610.8 succeeded
Details
typescript-eslint.typescript-eslint (Primary code validation and tests) Primary code validation and tests succeeded
Details
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_10_x) Run unit tests on other Node.js versions node_10_x succeeded
Details
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_6_x) Run unit tests on other Node.js versions node_6_x succeeded
Details
typescript-eslint.typescript-eslint (Run unit tests on other Node.js versions node_8_x) Run unit tests on other Node.js versions node_8_x succeeded
Details

@princjef princjef deleted the princjef:no-floating-promises branch Jun 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.