feat(no-unnecessary-collection): new rule (#281)

New rule to detect unnecessary usage of `combineLatest`, `forkJoin`,
etc. where a collection (object or array) containing only a single
observable is passed in. In these cases, `combineLatest`, etc. can be
replaced with the single observable itself (after accounting for slight
changes in output).

* This rule is added to the strict config.
* This rule does not include a fixer. A fixer could be implemented in
future for `merge`, `race`, `concat`, and the overload of
`combineLatest` that accepts an array.
* This rule does not support variable-length arrays and non-literal
objects because their lengths are not guaranteed.
* Like most rules in this plugin, this rule does not support import
aliases.

Resolves #39 and cartant#107 .

Author: JasonWeinzierl

README.md

@@ -147,6 +147,7 @@ The package includes the following rules.
 | [no-tap](docs/rules/no-tap.md)                                                   | Disallow the `tap` operator.                                                                              |      |    |    |    | ❌  |
 | [no-topromise](docs/rules/no-topromise.md)                                       | Disallow use of the `toPromise` method.                                                                   | ✅ 🔒 |    | 💡 | 💭 |    |
 | [no-unbound-methods](docs/rules/no-unbound-methods.md)                           | Disallow passing unbound methods.                                                                         | ✅ 🔒 |    |    | 💭 |    |
+| [no-unnecessary-collection](docs/rules/no-unnecessary-collection.md)             | Disallow unnecessary usage of collection arguments with single values.                                    | 🔒   |    |    |    |    |
 | [no-unsafe-catch](docs/rules/no-unsafe-catch.md)                                 | Disallow unsafe `catchError` usage in effects and epics.                                                  |      |    |    | 💭 |    |
 | [no-unsafe-first](docs/rules/no-unsafe-first.md)                                 | Disallow unsafe `first`/`take` usage in effects and epics.                                                |      |    |    | 💭 |    |
 | [no-unsafe-subject-next](docs/rules/no-unsafe-subject-next.md)                   | Disallow unsafe optional `next` calls.                                                                    | ✅ 🔒 |    |    | 💭 |    |

docs/rules/no-unnecessary-collection.md

@@ -0,0 +1,59 @@
+# Disallow unnecessary usage of collection arguments with single values (`rxjs-x/no-unnecessary-collection`)
+
+💼 This rule is enabled in the 🔒 `strict` config.
+
+<!-- end auto-generated rule header -->
+
+This rule effects failures when passing a collection (object or array) containing a single observable
+to the static observable creators that accept multiple observables
+(`combineLatest`, `forkJoin`, `merge`, `zip`, `concat`, `race`).
+Use of these creator functions with only a single observable
+can be replaced with direct usage of the observable itself.
+
+## Rule details
+
+Examples of **incorrect** code for this rule:
+
+```ts
+import { combineLatest, forkJoin, merge, zip, concat, race, of } from "rxjs";
+
+// These incorrect example can be simplified:
+const a$ = combineLatest([of(1)]);
+const b$ = forkJoin([of('data')]);
+
+const c$ = combineLatest({ value: of(1) });
+const d$ = forkJoin({ data: of('hello') });
+
+const e$ = merge(of(1));
+const f$ = zip(of(1));
+const g$ = concat(of(1));
+const h$ = race(of(1));
+```
+
+Examples of **correct** code for this rule:
+
+```ts
+import { of, map } from "rxjs";
+
+// These are equivalent to the previous examples:
+const a$ = of(1);
+const b$ = of('data').pipe(map(x => [x]));
+
+const c$ = of(1).pipe(map(x => ({ value: x })))
+const d$ = of('hello').pipe(map(x => ({ data: x })));
+
+const e$ = of(1);
+const f$ = of(1).pipe(map(x => [x]));
+const g$ = of(1);
+const h$ = of(1);
+```
+
+## When Not To Use It
+
+If you don't care about unnecessary usage of static observable creators,
+then you don't need this rule.
+
+## Resources
+
+- [Rule source](/src/rules/no-unnecessary-collection.ts)
+- [Test source](/tests/rules/no-unnecessary-collection.test.ts)

src/configs/strict.ts

@@ -33,6 +33,7 @@ export const createStrictConfig = (
     'rxjs-x/no-subscribe-in-pipe': 'error',
     'rxjs-x/no-topromise': 'error',
     'rxjs-x/no-unbound-methods': 'error',
+    'rxjs-x/no-unnecessary-collection': 'error',
     'rxjs-x/no-unsafe-subject-next': 'error',
     'rxjs-x/no-unsafe-takeuntil': 'error',
     'rxjs-x/prefer-observer': 'error',

src/constants.ts

@@ -9,6 +9,19 @@ export const SOURCES_OBJECT_ACCEPTING_STATIC_OBSERVABLE_CREATORS = [
   'forkJoin',
 ];
 
+/**
+ * The names of static observable creators
+ * that accept a collection (array or object) of multiple observables as input.
+ */
+export const MULTIPLE_OBSERVABLE_ACCEPTING_STATIC_OBSERVABLE_CREATORS = [
+  'combineLatest',
+  'forkJoin',
+  'merge',
+  'zip',
+  'concat',
+  'race',
+];
+
 /**
  * The names of types that are allowed to be passed unbound.
  */

src/index.ts

@@ -41,6 +41,7 @@ import { noSubscribeInPipeRule } from './rules/no-subscribe-in-pipe';
 import { noTapRule } from './rules/no-tap';
 import { noTopromiseRule } from './rules/no-topromise';
 import { noUnboundMethodsRule } from './rules/no-unbound-methods';
+import { noUnnecessaryCollectionRule } from './rules/no-unnecessary-collection';
 import { noUnsafeCatchRule } from './rules/no-unsafe-catch';
 import { noUnsafeFirstRule } from './rules/no-unsafe-first';
 import { noUnsafeSubjectNext } from './rules/no-unsafe-subject-next';
@@ -89,6 +90,7 @@ const allRules = {
   'no-tap': noTapRule,
   'no-topromise': noTopromiseRule,
   'no-unbound-methods': noUnboundMethodsRule,
+  'no-unnecessary-collection': noUnnecessaryCollectionRule,
   'no-unsafe-catch': noUnsafeCatchRule,
   'no-unsafe-first': noUnsafeFirstRule,
   'no-unsafe-subject-next': noUnsafeSubjectNext,

src/rules/no-unnecessary-collection.ts

@@ -0,0 +1,107 @@
+import {
+  TSESTree as es,
+} from '@typescript-eslint/utils';
+import { MULTIPLE_OBSERVABLE_ACCEPTING_STATIC_OBSERVABLE_CREATORS, SOURCES_OBJECT_ACCEPTING_STATIC_OBSERVABLE_CREATORS } from '../constants';
+import { getTypeServices, isArrayExpression, isObjectExpression, isProperty } from '../etc';
+import { ruleCreator } from '../utils';
+
+export const noUnnecessaryCollectionRule = ruleCreator({
+  defaultOptions: [],
+  meta: {
+    docs: {
+      description: 'Disallow unnecessary usage of collection arguments with single values.',
+      recommended: 'strict',
+      requiresTypeChecking: false,
+    },
+    messages: {
+      forbidden: 'Unnecessary {{operator}} with {{inputType}}. Use the observable directly instead.',
+    },
+    schema: [],
+    type: 'suggestion',
+  },
+  name: 'no-unnecessary-collection',
+  create: (context) => {
+    const { couldBeType, couldBeObservable } = getTypeServices(context);
+
+    function couldBeFromRxjs(node: es.Node, operatorName: string): boolean {
+      return couldBeType(node, operatorName, { name: /[/\\]rxjs[/\\]/ });
+    }
+
+    function checkCallExpression(node: es.CallExpression, operatorName: string): void {
+      const args = node.arguments;
+
+      if (args.length === 0) {
+        return;
+      }
+
+      const firstArg = args[0];
+
+      // Single-valued array.
+      if (isArrayExpression(firstArg)) {
+        const nonNullElements = firstArg.elements.filter(element => element !== null);
+        if (nonNullElements.length === 1 && couldBeObservable(nonNullElements[0])) {
+          context.report({
+            messageId: 'forbidden',
+            node: node.callee,
+            data: {
+              operator: operatorName,
+              inputType: 'single-valued array',
+            },
+          });
+        }
+        return;
+      }
+
+      // Single-property object.
+      if (isObjectExpression(firstArg) && SOURCES_OBJECT_ACCEPTING_STATIC_OBSERVABLE_CREATORS.includes(operatorName)) {
+        if (firstArg.properties.length === 1 && isProperty(firstArg.properties[0])) {
+          const property = firstArg.properties[0];
+          if (property.value && couldBeObservable(property.value)) {
+            context.report({
+              messageId: 'forbidden',
+              node: node.callee,
+              data: {
+                operator: operatorName,
+                inputType: 'single-property object',
+              },
+            });
+          }
+        }
+        return;
+      }
+
+      // Single rest parameter argument.
+      if (args.length === 1 && couldBeObservable(firstArg)) {
+        context.report({
+          messageId: 'forbidden',
+          node: node.callee,
+          data: {
+            operator: operatorName,
+            inputType: 'single argument',
+          },
+        });
+      }
+    }
+
+    const callExpressionVisitors: Record<string, (node: es.CallExpression) => void> = {};
+
+    for (const operator of MULTIPLE_OBSERVABLE_ACCEPTING_STATIC_OBSERVABLE_CREATORS) {
+      callExpressionVisitors[`CallExpression[callee.name="${operator}"]`] = (node: es.CallExpression) => {
+        if (couldBeFromRxjs(node.callee, operator)) {
+          checkCallExpression(node, operator);
+        }
+      };
+    }
+
+    for (const operator of MULTIPLE_OBSERVABLE_ACCEPTING_STATIC_OBSERVABLE_CREATORS) {
+      callExpressionVisitors[`CallExpression[callee.type="MemberExpression"][callee.property.name="${operator}"]`] = (node: es.CallExpression) => {
+        const memberExpr = node.callee as es.MemberExpression;
+        if (couldBeFromRxjs(memberExpr.property, operator)) {
+          checkCallExpression(node, operator);
+        }
+      };
+    }
+
+    return callExpressionVisitors;
+  },
+});