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 require-await #674

Merged
merged 3 commits into from Jul 19, 2019

Conversation

scottohara
Copy link
Contributor

@scottohara scottohara commented Jul 5, 2019

Extends the require-await rule from ESlint core to allow for async functions that explicitly return a promise (as opposed to a non-promise value that the async function implicitly wraps in Promise.resolve).

To recap, the main intent of the require-await rule in ESlint core is guard against marking a function as async unnecessarily. The presence of an await expression is the signal used to determine if async is necessary.

However, when used in conjunction with:

  • @typescript-eslint/promise-function-async, which requires all functions that return a promise to be marked as async; and
  • no-return-await, which warns against using return await

..there is an unresolvable conflict.

Fixes #669

Examples of newly allowed code

/*eslint require-await: "off"*/
/*eslint @typescript-eslint/require-await: "error"*/

async function numberOne(): Promise<number> {
  return Promise.resolve(1);
}

The above function would normally generate a warning from require-await:

Async function ‘numberOne’ has no `await` expression.

Similarly:

async function numberOne(): Promise<number> {
  return getAsyncNumber(1);
}

async function getAsyncNumber(x: number): Promise<number> {
  return Promise.resolve(x);
}

The above async function numberOne returns the result of another async function getAsyncNumber, which would normally trigger two warnings.

Aside from this one specific case, the rule passes through to ESLint core require-await for everything else, so the following still applies:

Examples of valid code

// Non-async function declaration
function numberOne(): number {
  return 1;
}

// Non-async function expression
const numberOne = function(): number {
  return 1;
}

// Non-async arrow function expression
const numberOne = (): number => 1;

// Async function declaration with await
async function numberOne(): Promise<number> {
  return await 1;
}

// Async function expression with await
const numberOne = async function(): Promise<number> {
  return await 1;
}

// Async arrow function expression with await
const numberOne = async (): Promise<number> => await 1;

Examples of invalid code

// Async function declaration with no await
async function numberOne(): Promise<number> {
  return 1;
}

// Async function expression with no await
const numberOne = async function(): Promise<number> {
  return 1;
}

// Async arrow function expression with no await
const numberOne = async (): Promise<number> => 1;

@codecov
Copy link

codecov bot commented Jul 5, 2019

Codecov Report

Merging #674 into master will decrease coverage by 0.01%.
The diff coverage is 92.3%.

@@            Coverage Diff             @@
##           master     #674      +/-   ##
==========================================
- Coverage   94.43%   94.41%   -0.02%     
==========================================
  Files         112      113       +1     
  Lines        4672     4711      +39     
  Branches     1286     1293       +7     
==========================================
+ Hits         4412     4448      +36     
- Misses        149      150       +1     
- Partials      111      113       +2
Impacted Files Coverage Δ
packages/eslint-plugin/src/rules/index.ts 100% <100%> (ø) ⬆️
packages/eslint-plugin/src/rules/require-await.ts 92.1% <92.1%> (ø)

JamesHenry
JamesHenry previously approved these changes Jul 12, 2019
Copy link
Member

@JamesHenry JamesHenry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nicely done, thank you!

@JamesHenry JamesHenry added 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 package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin labels Jul 12, 2019
Copy link
Member

@bradzacher bradzacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

few nits, otherwise LGTM.
Thanks for doing this, this is a great addition.

packages/eslint-plugin/src/rules/require-await.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/tests/rules/require-await.test.ts Outdated Show resolved Hide resolved
packages/eslint-plugin/tests/rules/require-await.test.ts Outdated Show resolved Hide resolved
@bradzacher
Copy link
Member

unless github is glitching out on me, it looks like your push didn't push any changes.

@scottohara
Copy link
Contributor Author

Sorry @bradzacher, minor brain explosion here. Should be OK now.

@bradzacher bradzacher merged commit 807bc2d into typescript-eslint:master Jul 19, 2019
@scottohara scottohara deleted the require-await branch August 8, 2019 06:29
@fregante
Copy link
Contributor

fregante commented Aug 29, 2019

I think this rule missed await-less arrow functions:

OK

async function foo() {
	return Promise.resolve();
}

Not OK

const foo = () => Promise.resolve();

@scottohara
Copy link
Contributor Author

scottohara commented Aug 29, 2019

@fregante

The arrow function use case should be addressed in #826. (merged, not yet released)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2020
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 package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rule proposal: typescript-eslint version of the base [require-await] rule
4 participants