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

Async rule changes #156

Merged
merged 12 commits into from
Sep 26, 2024
Merged

Async rule changes #156

merged 12 commits into from
Sep 26, 2024

Conversation

jattasNI
Copy link
Collaborator

@jattasNI jattasNI commented Sep 19, 2024

Rationale

Resolves #151. This makes it so a promise must be awaited (or otherwise handled, just not ignored) in Angular tests, making them match other code.

Resolves #115. This makes it so a function marked async must await something, not contain only synchronous code and not return a promise.

Implementation

  1. Stop disabling no-floating-promises in Angular tests. See PR comment for rationale for keeping no-unbound-method: 'off' here.
    • this repo even had some Angular tests that violated this rule because they were using stale jasminewd2 types. I fixed those by using jasmine types instead (and fixing a playwright test that was incorrectly referencing jasmine types)
    • this required adding an .npmrc since my dev machine is configured to use Artifactory
  2. Set no-return-await: 'off' in the JS ruleset. This rule is now deprecated by ESLint because returning an awaited promise is no longer a performance concern.
    • the related @typescript-eslint/return-await': 'error' was already enabled and is still useful because it enforces the inverse, that you should await promises rather than just return them because you get better stack traces
  3. Set 'require-await': 'error' in the JS ruleset and '@typescript-eslint/require-await': 'error' in the TS-with-typechecking ruleset.
  4. Change files for beachball marked minor in accordance with CONTRIBUTING.md guidance for rule changes and include package-specific descriptions of the changes

Testing

A draft PR into systemlink-lib-angular shows the impact of some of these changes. It requires mostly mechanical changes and catches lots of legitimate issues.

I also updated a couple test files in this repo to include more async cases and played around with those implementations locally to verify the rules were working.

@jattasNI jattasNI marked this pull request as ready for review September 19, 2024 22:27
tests/javascript/index.js Outdated Show resolved Hide resolved
tests/typescript-requiring-type-checking/index.ts Outdated Show resolved Hide resolved
tests/typescript-requiring-type-checking/index.ts Outdated Show resolved Hide resolved
@rajsite rajsite self-requested a review September 20, 2024 22:12
@rajsite rajsite merged commit 2fbb47b into main Sep 26, 2024
2 checks passed
@rajsite rajsite deleted the async-rule-changes branch September 26, 2024 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enable no-floating-promises for Angular tests Consider enabling the require-await rule
4 participants