feat(eslint-plugin): add avoid-combining-component-store-selectors ru…

…le (#4043)
ngrx · Sep 17, 2023 · 0bff440 · 0bff440
1 parent f2514ba
commit 0bff440
Show file tree

Hide file tree

Showing 13 changed files with 385 additions and 7 deletions.
diff --git a/modules/eslint-plugin/spec/rules/avoid-combining-component-store-selectors.spec.ts b/modules/eslint-plugin/spec/rules/avoid-combining-component-store-selectors.spec.ts
@@ -0,0 +1,215 @@
+import type {
+  ESLintUtils,
+  TSESLint,
+} from '@typescript-eslint/experimental-utils';
+import { fromFixture } from 'eslint-etc';
+import * as path from 'path';
+import rule, {
+  messageId,
+} from '../../src/rules/component-store/avoid-combining-component-store-selectors';
+import { ruleTester } from '../utils';
+
+type MessageIds = ESLintUtils.InferMessageIdsTypeFromRule<typeof rule>;
+type Options = ESLintUtils.InferOptionsTypeFromRule<typeof rule>;
+type RunTests = TSESLint.RunTests<MessageIds, Options>;
+
+const validConstructor: () => RunTests['valid'] = () => [
+  `
+import { ComponentStore } from '@ngrx/component-store'
+class Ok extends ComponentStore<MoviesState> {
+  movies$ = this.select((state) => state.movies);
+  selectedId$ = this.select((state) => state.selectedId);
+  movie$ = this.select(
+    this.movies$,
+    this.selectedId$,
+    ([movies, selectedId]) => movies[selectedId]
+  );
+
+  constructor() {
+    super({ movies: [] })
+  }
+}`,
+  `
+import { ComponentStore } from '@ngrx/component-store'
+class Ok {
+  readonly movies$ = this.store.select((state) => state.movies);
+  readonly selectedId$ = this.store.select((state) => state.selectedId);
+  readonly movie$ = this.store.select(
+    this.movies$,
+    this.selectedId$,
+    ([movies, selectedId]) => movies[selectedId]
+  );
+
+  constructor(private readonly store: ComponentStore<MoviesState>) {}
+}`,
+  `
+import { ComponentStore } from '@ngrx/component-store'
+class Ok {
+  movie$: Observable<unknown>
+
+  constructor(customStore: ComponentStore<MoviesState>) {
+    const movies = customStore.select((state) => state.movies);
+    const selectedId = this.customStore.select((state) => state.selectedId);
+    
+    this.movie$ = this.customStore.select(
+      this.movies$,
+      this.selectedId$,
+      ([movies, selectedId]) => movies[selectedId]
+    );
+  }
+}`,
+  `
+import { ComponentStore } from '@ngrx/component-store'
+class Ok {
+  vm$ = combineLatest(this.somethingElse(), this.customStore.select(selectItems))
+
+  constructor(customStore: ComponentStore<MoviesState>) {}
+}`,
+  `
+import { ComponentStore } from '@ngrx/component-store'
+class Ok extends ComponentStore<MoviesState> {
+  vm$ = combineLatest(this.select(selectItems), this.somethingElse())
+}`,
+];
+
+const validInject: () => RunTests['valid'] = () => [
+  `
+import { inject } from '@angular/core'
+import { ComponentStore } from '@ngrx/component-store'
+class Ok {
+  readonly store = inject(ComponentStore<MoviesState>)
+  readonly movies$ = this.store.select((state) => state.movies);
+  readonly selectedId$ = this.store.select((state) => state.selectedId);
+  readonly movie$ = this.store.select(
+    this.movies$,
+    this.selectedId$,
+    ([movies, selectedId]) => movies[selectedId]
+  );
+}`,
+  `
+import { inject } from '@angular/core'
+import { ComponentStore } from '@ngrx/component-store'
+class Ok {
+  readonly store = inject(ComponentStore<MoviesState>)
+  readonly vm$ = combineLatest(this.store.select(selectItems), somethingElse())
+}`,
+];
+
+const invalidConstructor: () => RunTests['invalid'] = () => [
+  fromFixture(`
+import { ComponentStore } from '@ngrx/component-store'
+
+class NotOk extends ComponentStore<MoviesState> {
+  movie$ = combineLatest(
+    this.select((state) => state.movies),
+    this.select((state) => state.selectedId),
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [${messageId}]
+  );
+
+  constructor() {
+    super({ movies: [] })
+  }
+}`),
+  fromFixture(`
+import { ComponentStore } from '@ngrx/component-store'
+
+class NotOk extends ComponentStore<MoviesState> {
+  movie$ = combineLatest(
+    this.select((state) => state.movies),
+    this.select((state) => state.selectedId),
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [${messageId}]
+    this.select((state) => state.selectedId),
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [${messageId}]
+  );
+
+  constructor() {
+    super({ movies: [] })
+  }
+}`),
+  fromFixture(`
+import { ComponentStore } from '@ngrx/component-store'
+class NotOk {
+  movie$ = combineLatest(
+    this.moviesState.select((state) => state.movies),
+    this.moviesState.select((state) => state.selectedId),
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [${messageId}]
+  );
+
+  constructor(private readonly moviesState: ComponentStore<MoviesState>) {}
+}`),
+  fromFixture(`
+import { ComponentStore } from '@ngrx/component-store'
+class NotOk {
+  movie$ = combineLatest(
+    this.moviesState.select((state) => state.movies),
+    this.moviesState.select((state) => state.selectedId),
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [${messageId}]
+    this.moviesState.select((state) => state.selectedId),
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [${messageId}]
+  );
+
+  constructor(private readonly moviesState: ComponentStore<MoviesState>) {}
+}`),
+  fromFixture(`
+import { ComponentStore } from '@ngrx/component-store'
+class NotOk {
+  movie$: Observable<unknown>
+
+  constructor(store: ComponentStore<MoviesState>) {
+    this.movie$ = combineLatest(
+      store.select((state) => state.movies),
+      store.select((state) => state.selectedId)
+      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [${messageId}]
+    );
+  }
+}
+`),
+  fromFixture(`
+import { ComponentStore } from '@ngrx/component-store'
+class NotOk {
+  movie$: Observable<unknown>
+
+  constructor(store: ComponentStore<MoviesState>) {
+    this.movie$ = combineLatest(
+      store.select((state) => state.movies),
+      store.select((state) => state.selectedId),
+      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [${messageId}]
+      store.select((state) => state.selectedId)
+      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [${messageId}]
+    );
+  }
+}
+`),
+];
+
+const invalidInject: () => RunTests['invalid'] = () => [
+  fromFixture(`
+import { inject } from '@angular/core'
+import { ComponentStore } from '@ngrx/component-store'
+class NotOk {
+  readonly componentStore = inject(ComponentStore<MoviesState>)
+  readonly movie$ = combineLatest(
+    this.componentStore.select((state) => state.movies),
+    this.componentStore.select((state) => state.selectedId),
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [${messageId}]
+  );
+}`),
+  fromFixture(`
+import { inject } from '@angular/core'
+import { ComponentStore } from '@ngrx/component-store'
+class NotOk {
+  readonly store = inject(ComponentStore<MoviesState>)
+  readonly movie$ = combineLatest(
+    this.store.select((state) => state.movies),
+    this.store.select((state) => state.selectedId),
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [${messageId}]
+    this.store.select((state) => state.selectedId),
+    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ [${messageId}]
+  );
+}`),
+];
+
+ruleTester().run(path.parse(__filename).name, rule, {
+  valid: [...validConstructor(), ...validInject()],
+  invalid: [...invalidConstructor(), ...invalidInject()],
+});
diff --git a/modules/eslint-plugin/spec/rules/avoid-combining-selectors.spec.ts b/modules/eslint-plugin/spec/rules/avoid-combining-selectors.spec.ts
@@ -84,7 +84,7 @@ import { inject } from '@angular/core'
 
 class Ok7 {
   readonly store = inject(Store)
-  vm$ = combineLatest(this.store$.select(selectItems), this.somethingElse())
+  vm$ = combineLatest(this.store.select(selectItems), this.somethingElse())
 }`,
   `
 import { Store } from '@ngrx/store'

diff --git a/modules/eslint-plugin/src/configs/all-requiring-type-checking.ts b/modules/eslint-plugin/src/configs/all-requiring-type-checking.ts
@@ -12,6 +12,7 @@ export = {
   },
   plugins: ['@ngrx'],
   rules: {
+    '@ngrx/avoid-combining-component-store-selectors': 'warn',
     '@ngrx/updater-explicit-return-type': 'warn',
     '@ngrx/avoid-cyclic-effects': 'warn',
     '@ngrx/no-dispatch-in-effects': 'warn',

diff --git a/modules/eslint-plugin/src/configs/all.ts b/modules/eslint-plugin/src/configs/all.ts
@@ -8,6 +8,7 @@ export = {
 
   plugins: ['@ngrx'],
   rules: {
+    '@ngrx/avoid-combining-component-store-selectors': 'warn',
     '@ngrx/updater-explicit-return-type': 'warn',
     '@ngrx/no-dispatch-in-effects': 'warn',
     '@ngrx/no-effects-in-providers': 'error',

diff --git a/modules/eslint-plugin/src/configs/component-store-strict.ts b/modules/eslint-plugin/src/configs/component-store-strict.ts
@@ -7,5 +7,8 @@ export = {
   parser: '@typescript-eslint/parser',
 
   plugins: ['@ngrx'],
-  rules: { '@ngrx/updater-explicit-return-type': 'error' },
+  rules: {
+    '@ngrx/avoid-combining-component-store-selectors': 'error',
+    '@ngrx/updater-explicit-return-type': 'error',
+  },
 };
diff --git a/modules/eslint-plugin/src/configs/component-store.ts b/modules/eslint-plugin/src/configs/component-store.ts
@@ -7,5 +7,8 @@ export = {
   parser: '@typescript-eslint/parser',
 
   plugins: ['@ngrx'],
-  rules: { '@ngrx/updater-explicit-return-type': 'warn' },
+  rules: {
+    '@ngrx/avoid-combining-component-store-selectors': 'warn',
+    '@ngrx/updater-explicit-return-type': 'warn',
+  },
 };
diff --git a/modules/eslint-plugin/src/configs/recommended-requiring-type-checking.ts b/modules/eslint-plugin/src/configs/recommended-requiring-type-checking.ts
@@ -12,6 +12,7 @@ export = {
   },
   plugins: ['@ngrx'],
   rules: {
+    '@ngrx/avoid-combining-component-store-selectors': 'warn',
     '@ngrx/updater-explicit-return-type': 'warn',
     '@ngrx/avoid-cyclic-effects': 'warn',
     '@ngrx/no-dispatch-in-effects': 'warn',

diff --git a/modules/eslint-plugin/src/configs/recommended.ts b/modules/eslint-plugin/src/configs/recommended.ts
@@ -8,6 +8,7 @@ export = {
 
   plugins: ['@ngrx'],
   rules: {
+    '@ngrx/avoid-combining-component-store-selectors': 'warn',
     '@ngrx/updater-explicit-return-type': 'warn',
     '@ngrx/no-dispatch-in-effects': 'warn',
     '@ngrx/no-effects-in-providers': 'error',

diff --git a/modules/eslint-plugin/src/configs/strict-requiring-type-checking.ts b/modules/eslint-plugin/src/configs/strict-requiring-type-checking.ts
@@ -12,6 +12,7 @@ export = {
   },
   plugins: ['@ngrx'],
   rules: {
+    '@ngrx/avoid-combining-component-store-selectors': 'error',
     '@ngrx/updater-explicit-return-type': 'error',
     '@ngrx/avoid-cyclic-effects': 'error',
     '@ngrx/no-dispatch-in-effects': 'error',

diff --git a/modules/eslint-plugin/src/configs/strict.ts b/modules/eslint-plugin/src/configs/strict.ts
@@ -8,6 +8,7 @@ export = {
 
   plugins: ['@ngrx'],
   rules: {
+    '@ngrx/avoid-combining-component-store-selectors': 'error',
     '@ngrx/updater-explicit-return-type': 'error',
     '@ngrx/no-dispatch-in-effects': 'error',
     '@ngrx/no-effects-in-providers': 'error',

diff --git a/modules/eslint-plugin/src/rules/component-store/avoid-combining-component-store-selectors.ts b/modules/eslint-plugin/src/rules/component-store/avoid-combining-component-store-selectors.ts
@@ -0,0 +1,58 @@
+import type { TSESTree } from '@typescript-eslint/experimental-utils';
+import * as path from 'path';
+import { createRule } from '../../rule-creator';
+import {
+  asPattern,
+  getNgRxComponentStores,
+  namedExpression,
+} from '../../utils';
+export const messageId = 'avoidCombiningComponentStoreSelectors';
+type MessageIds = typeof messageId;
+type Options = readonly [];
+
+export default createRule<Options, MessageIds>({
+  name: path.parse(__filename).name,
+  meta: {
+    type: 'suggestion',
+    ngrxModule: 'component-store',
+    docs: {
+      description: 'Prefer combining selectors at the selector level.',
+      recommended: 'warn',
+    },
+    schema: [],
+    messages: {
+      [messageId]: 'Combine selectors at the selector level.',
+    },
+  },
+  defaultOptions: [],
+  create: (context) => {
+    const { identifiers = [] } = getNgRxComponentStores(context);
+    const storeNames = identifiers.length > 0 ? asPattern(identifiers) : null;
+
+    const thisSelects = `CallExpression[callee.object.type='ThisExpression'][callee.property.name='select']`;
+    const storeSelects = storeNames ? namedExpression(storeNames) : null;
+
+    const selectsInArray: TSESTree.CallExpression[] = [];
+    return {
+      [`ClassDeclaration[superClass.name=/Store/] CallExpression[callee.name='combineLatest'] ${thisSelects} ~ ${thisSelects}`](
+        node: TSESTree.CallExpression
+      ) {
+        selectsInArray.push(node);
+      },
+      [`CallExpression[callee.name='combineLatest'] ${storeSelects} ~ ${storeSelects}`](
+        node: TSESTree.CallExpression
+      ) {
+        selectsInArray.push(node);
+      },
+      [`CallExpression[callee.name='combineLatest']:exit`]() {
+        for (const node of selectsInArray) {
+          context.report({
+            node,
+            messageId,
+          });
+        }
+        selectsInArray.length = 0;
+      },
+    };
+  },
+});
diff --git a/projects/ngrx.io/content/guide/eslint-plugin/index.md b/projects/ngrx.io/content/guide/eslint-plugin/index.md
@@ -15,9 +15,10 @@ Some rules also allow automatic fixes with `ng lint --fix`.
 
 ### component-store
 
-| Name                                                                                          | Description                                    | Recommended | Category | Fixable | Has suggestions | Configurable | Requires type information |
-| --------------------------------------------------------------------------------------------- | ---------------------------------------------- | ----------- | -------- | ------- | --------------- | ------------ | ------------------------- |
-| [@ngrx/updater-explicit-return-type](/guide/eslint-plugin/rules/updater-explicit-return-type) | `Updater` should have an explicit return type. | problem     | warn     | No      | No              | No           | No                        |
+| Name                                                                                                                    | Description                                       | Recommended | Category | Fixable | Has suggestions | Configurable | Requires type information |
+| ----------------------------------------------------------------------------------------------------------------------- | ------------------------------------------------- | ----------- | -------- | ------- | --------------- | ------------ | ------------------------- |
+| [@ngrx/avoid-combining-component-store-selectors](/guide/eslint-plugin/rules/avoid-combining-component-store-selectors) | Prefer combining selectors at the selector level. | suggestion  | warn     | No      | No              | No           | No                        |
+| [@ngrx/updater-explicit-return-type](/guide/eslint-plugin/rules/updater-explicit-return-type)                           | `Updater` should have an explicit return type.    | problem     | warn     | No      | No              | No           | No                        |
 
 ### effects
 
@@ -63,7 +64,7 @@ Some rules also allow automatic fixes with `ng lint --fix`.
 <!-- CONFIGURATIONS-CONFIG:START -->
 
 | Name                                                                                                                                                             |
-|------------------------------------------------------------------------------------------------------------------------------------------------------------------|
+| ---------------------------------------------------------------------------------------------------------------------------------------------------------------- |
 | [all-requiring-type-checking](https://github.com/ngrx/platform/blob/main/modules/eslint-plugin/src/configs/all-requiring-type-checking.ts)                       |
 | [all](https://github.com/ngrx/platform/blob/main/modules/eslint-plugin/src/configs/all.ts)                                                                       |
 | [component-store-strict](https://github.com/ngrx/platform/blob/main/modules/eslint-plugin/src/configs/component-store-strict.ts)                                 |