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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(eslint-config): enable return-await #4222

Merged
merged 2 commits into from
Dec 3, 2019
Merged

Conversation

bajtos
Copy link
Member

@bajtos bajtos commented Nov 28, 2019

Require users to await a promise before returning the result inside a try block. Otherwise the errors won't be caught by the catch block.

Require users to NOT await a promise when the result is immediately returned outside of a try block. This is a (micro)performance optimization to avoid unnecessary allocation of an extra Promise instance.

Rule docs: https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/return-await.md

At the moment (2019-11-28), eslint reports false positives, they should be fixed by typescript-eslint/typescript-eslint#1270. This pull request cannot be landed until that patch is released & our package-lock files are updated with the new version of the eslint plugin.
The eslint fix was released in v2.10.0.

Checklist

馃憠 Read and sign the CLA (Contributor License Agreement) 馃憟

  • npm test passes on your machine
  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style guide
  • API Documentation in code was updated
  • Documentation in /docs/site was updated
  • Affected artifact templates in packages/cli were updated
  • Affected example projects in examples/* were updated

馃憠 Check out how to submit a PR 馃憟

@bajtos bajtos added the Internal Tooling Issues related to our tooling and monorepo infrastructore label Nov 28, 2019
@bajtos bajtos self-assigned this Nov 28, 2019
@bajtos
Copy link
Member Author

bajtos commented Nov 28, 2019

@raymondfeng @strongloop/loopback-next WDYT, should we release this change as semver-major?

@bajtos bajtos added the blocked label Nov 28, 2019
Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

馃憤 Agreed.

LGTM after fixing the existing code that breaks the rule.

@raymondfeng
Copy link
Contributor

Please fix the following violations:

/home/travis/build/strongloop/loopback-next/examples/context/src/custom-configuration-resolver.ts
  47:7  error  returning an awaited promise is required in this context  @typescript-eslint/return-await
/home/travis/build/strongloop/loopback-next/packages/boot/src/__tests__/fixtures/interceptor.artifact.ts
  49:7  error  returning an awaited promise is required in this context  @typescript-eslint/return-await
/home/travis/build/strongloop/loopback-next/packages/boot/src/__tests__/fixtures/non-global-interceptor.artifact.ts
  49:7  error  returning an awaited promise is required in this context  @typescript-eslint/return-await
/home/travis/build/strongloop/loopback-next/packages/cli/lib/base-generator.js
  74:7  error  returning an awaited promise is required in this context  @typescript-eslint/return-await
/home/travis/build/strongloop/loopback-next/packages/cli/test/snapshot-matcher.js
  170:5  error  returning an awaited promise is required in this context  @typescript-eslint/return-await
/home/travis/build/strongloop/loopback-next/packages/context/src/__tests__/acceptance/interceptor.acceptance.ts
  696:7  error  returning an awaited promise is required in this context  @typescript-eslint/return-await
/home/travis/build/strongloop/loopback-next/packages/repository/src/query.ts
  663:7  error  returning an awaited promise is required in this context  @typescript-eslint/return-await
/home/travis/build/strongloop/loopback-next/packages/rest/src/body-parsers/body-parser.ts
  64:9  error  returning an awaited promise is required in this context  @typescript-eslint/return-await
/home/travis/build/strongloop/loopback-next/packages/rest/src/coercion/coerce-parameter.ts
  191:5  error  returning an awaited promise is required in this context  @typescript-eslint/return-await

@bajtos
Copy link
Member Author

bajtos commented Dec 3, 2019

Please fix the following violations

See the PR description:

At the moment (2019-11-28), eslint reports false positives, they should be fixed by typescript-eslint/typescript-eslint#1270. This pull request cannot be landed until that patch is released & our package-lock files are updated with the new version of the eslint plugin.

The eslint fix was released in v2.10.0, we are good to proceed now.

@bajtos
Copy link
Member Author

bajtos commented Dec 3, 2019

Hmm, it looks like there are still some false positives reported by eslint:

/home/travis/build/strongloop/loopback-next/extensions/metrics/src/interceptors/metrics.interceptor.ts
88:7  error  returning an awaited value that is not a promise is not allowed  @typescript-eslint/return-await

The code:

https://github.com/strongloop/loopback-next/blob/4478f736e39aef68789834c8e77d4f3f7d933f68/extensions/metrics/src/interceptors/metrics.interceptor.ts#L86-L89

Strangely enough, I am not able able to reproduce this on my local machine any more 馃

BREAKING CHANGE: The linter will reject code using `return await`
ouside of `try` blocks or forgetting to `await` before returning
from inside a `try` block.
Migration guide: use `return` outside of `try` blocks and `return await`
inside `try` blocks.

Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
This removes certain kind of linting errors that are reported on Travis
but cannot be reproduced in a typical local setup where all packages
are bootstrapped.

Signed-off-by: Miroslav Bajto拧 <mbajtoss@gmail.com>
@bajtos
Copy link
Member Author

bajtos commented Dec 3, 2019

The problem mentioned by my previous comment was caused by the fact that on Travis CI, we did not bootstrap all packages. I am assuming that as a result, typescript-eslint did not have access to dependencies and therefore it used incorrect typings.

In 2e35a44, I have modified our Travis CI job to run a full bootstrap before calling eslint, this seems to fix the problem on my local machine.

@bajtos bajtos merged commit 5b5e561 into master Dec 3, 2019
@bajtos bajtos deleted the feat/eslint-return-await branch December 3, 2019 16:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Internal Tooling Issues related to our tooling and monorepo infrastructore
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants