Async rule changes (#156)

# 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
3. Set `no-return-await: 'off'` in the JS ruleset. This [rule is now
deprecated by
ESLint](https://eslint.org/docs/latest/rules/no-return-await) 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](https://typescript-eslint.io/rules/return-await/), that you
should await promises rather than just return them because you get
better stack traces
4. Set `'require-await': 'error'` in the JS ruleset and
`'@typescript-eslint/require-await': 'error'` in the
TS-with-typechecking ruleset.
5. 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](https://dev.azure.com/ni/DevCentral/_git/Skyline/pullrequest/761645)
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.

---------

Co-authored-by: Milan Raj <[email protected]>

.gitignore

@@ -2,6 +2,5 @@ node_modules
 .DS_Store
 *.tgz
 .vscode
-.npmrc
 ./rules.txt
 ./rules-diff.txt

.npmrc

@@ -0,0 +1 @@
+registry=https://registry.npmjs.org/

change/@ni-eslint-config-angular-225ce3da-b49d-4df7-ace0-b99abb8fbcb0.json

@@ -0,0 +1,7 @@
+{
+  "type": "minor",
+  "comment": "Rule changes for asynchronous code:\\n 1. Functions marked `async` must now include an `await`. The rule `'@typescript-eslint/require-await'` changed from `'off'` to `'error'`.\\n 2. Test code must now correctly handle promises. The rule `'@typescript-eslint/no-floating-promises'` changed from `'off'` to `'error'`.",
+  "packageName": "@ni/eslint-config-angular",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}

change/@ni-eslint-config-javascript-4569f2fb-83ed-4863-9f95-96f907ede343.json

@@ -0,0 +1,7 @@
+{
+  "type": "minor",
+  "comment": "Rule changes for asynchronous code:\\n 1. Functions marked `async` must now include an `await`. The rule `'require-await'` changed from `'off'` to `'error'`.\\n 2. It is now acceptable for a function to return an awaited promise. The rule `'no-return-await'` changed from `'error'` to `'off'`.",
+  "packageName": "@ni/eslint-config-javascript",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}

change/@ni-eslint-config-typescript-32797652-8b30-47bc-b259-dfe598d5a4e3.json

@@ -0,0 +1,7 @@
+{
+  "type": "minor",
+  "comment": "Rule changes for asynchronous code:\\n 1. Functions marked `async` must now include an `await`. The rule `'@typescript-eslint/require-await'` changed from `'off'` to `'error'`.",
+  "packageName": "@ni/eslint-config-typescript",
+  "email": "[email protected]",
+  "dependentChangeType": "patch"
+}

packages/eslint-config-angular/requiring-type-checking.js

@@ -8,10 +8,9 @@ module.exports = {
             files: ['*.spec.ts'],
             rules: {
                 /*
-                    The jasminewd2 library used by Angular applications results in a significant number of
-                    floating promises and unbound methods so these rules are disabled for test specs in Angular projects.
+                    Spies used by Angular application tests result in a significant number of
+                    unbound methods so this rule is disabled for test specs in Angular projects.
                 */
-                '@typescript-eslint/no-floating-promises': 'off',
                 '@typescript-eslint/unbound-method': 'off',
             }
         },

packages/eslint-config-javascript/index.js

@@ -325,6 +325,11 @@ module.exports = {
                 selector: 'CallExpression[callee.object.name=\'console\'][callee.property.name=/^(debug|info|time|timeEnd|trace)$/]',
                 message: 'Unexpected property on console object was called'
             }],
+        /*
+            This rule is deprecated since ESLint 8.46.0 because returning an awaited value no longer generates an extra microtask.
+            https://eslint.org/docs/latest/rules/no-return-await
+        */
+        'no-return-await': 'off',
 
         /*
             Disallow returning values from setters.
@@ -424,6 +429,11 @@ module.exports = {
         */
         'prefer-regex-literals': 'error',
 
+        /*
+            Asynchronous functions that don’t use await might not need to be asynchronous functions and could be the unintentional result of refactoring.
+        */
+        'require-await': 'error',
+
         /*
             This configuration already supports the JSDoc syntax. Add additional syntax as line or
             block exceptions or markers when necessary.

packages/eslint-config-typescript/lib/extensions-requiring-type-checking.js

@@ -12,12 +12,12 @@ module.exports = {
         'no-throw-literal': 'off',
         '@typescript-eslint/no-throw-literal': 'error',
 
-        // Defined by Airbnb
+        // Defined by NI
         'require-await': 'off',
-        '@typescript-eslint/require-await': 'off',
+        '@typescript-eslint/require-await': 'error',
 
         // Defined by Airbnb
         'no-return-await': 'off',
-        '@typescript-eslint/return-await': 'error',
+        '@typescript-eslint/return-await': ['error', 'always'],
     }
 };

tests/angular/package.json

@@ -13,6 +13,6 @@
     "@angular/core": "^17.3.12"
   },
   "devDependencies": {
-    "@types/jasminewd2": "^2.0.13"
+    "@types/jasmine": "^5.1.4"
   }
-}
+}

tests/javascript/index.js

@@ -12,4 +12,8 @@ export class NI {
     makeAwesomer() {
         this._awesomeLevel += 1;
     }
+
+    async asyncAwesomeness() {
+        await Promise.resolve(42);
+    }
 }

tests/playwright-requiring-type-checking/index.ts

@@ -1,6 +1,6 @@
 import { test, expect } from '@playwright/test';
 
-describe('AssetDisplayPropertiesSidenavWrapperComponent', () => {
+test.describe('AssetDisplayPropertiesSidenavWrapperComponent', () => {
     test('has title', async ({ page }) => {
         await page.goto('https://playwright.dev/');
 

tests/typescript-requiring-type-checking/index.ts

@@ -10,4 +10,10 @@ export class NI {
     public makeAwesomer(): void {
         this._awesomeLevel += 1;
     }
+
+    public async slowAdd(a: number, b: number, timeMs: number): Promise<number> {
+        return await new Promise(resolve => {
+            setTimeout(() => resolve(a + b), timeMs);
+        });
+    }
 }