From 0bff440896fe14d1618e708b240704ade272c1be Mon Sep 17 00:00:00 2001 From: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com> Date: Sun, 17 Sep 2023 19:22:29 +0200 Subject: [PATCH] feat(eslint-plugin): add avoid-combining-component-store-selectors rule (#4043) --- ...ombining-component-store-selectors.spec.ts | 215 ++++++++++++++++++ .../rules/avoid-combining-selectors.spec.ts | 2 +- .../configs/all-requiring-type-checking.ts | 1 + modules/eslint-plugin/src/configs/all.ts | 1 + .../src/configs/component-store-strict.ts | 5 +- .../src/configs/component-store.ts | 5 +- .../recommended-requiring-type-checking.ts | 1 + .../eslint-plugin/src/configs/recommended.ts | 1 + .../configs/strict-requiring-type-checking.ts | 1 + modules/eslint-plugin/src/configs/strict.ts | 1 + ...oid-combining-component-store-selectors.ts | 58 +++++ .../content/guide/eslint-plugin/index.md | 9 +- ...oid-combining-component-store-selectors.md | 92 ++++++++ 13 files changed, 385 insertions(+), 7 deletions(-) create mode 100644 modules/eslint-plugin/spec/rules/avoid-combining-component-store-selectors.spec.ts create mode 100644 modules/eslint-plugin/src/rules/component-store/avoid-combining-component-store-selectors.ts create mode 100644 projects/ngrx.io/content/guide/eslint-plugin/rules/avoid-combining-component-store-selectors.md 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 new file mode 100644 index 0000000000..95d2b561cd --- /dev/null +++ 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; +type Options = ESLintUtils.InferOptionsTypeFromRule; +type RunTests = TSESLint.RunTests; + +const validConstructor: () => RunTests['valid'] = () => [ + ` +import { ComponentStore } from '@ngrx/component-store' +class Ok extends ComponentStore { + 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) {} +}`, + ` +import { ComponentStore } from '@ngrx/component-store' +class Ok { + movie$: Observable + + constructor(customStore: ComponentStore) { + 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) {} +}`, + ` +import { ComponentStore } from '@ngrx/component-store' +class Ok extends ComponentStore { + 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) + 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) + readonly vm$ = combineLatest(this.store.select(selectItems), somethingElse()) +}`, +]; + +const invalidConstructor: () => RunTests['invalid'] = () => [ + fromFixture(` +import { ComponentStore } from '@ngrx/component-store' + +class NotOk extends ComponentStore { + 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 { + 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) {} +}`), + 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) {} +}`), + fromFixture(` +import { ComponentStore } from '@ngrx/component-store' +class NotOk { + movie$: Observable + + constructor(store: ComponentStore) { + 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 + + constructor(store: ComponentStore) { + 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) + 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) + 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 index 847a4b8b24..ab81d7e8e1 100644 --- 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 index 5fc1875976..7a031377dd 100644 --- 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 index 6e38a5ac5e..ed85355bc5 100644 --- 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 index f1b605182c..e2e8e17140 100644 --- 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 index c2f027468a..af3d6de23f 100644 --- 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 index 5fc1875976..7a031377dd 100644 --- 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 index 6e38a5ac5e..ed85355bc5 100644 --- 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 index 189c0b48ab..8ed2f3cd21 100644 --- 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 index 4df5cc5776..04ceac8458 100644 --- 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 new file mode 100644 index 0000000000..5c00e3c1aa --- /dev/null +++ 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({ + 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 index 7c584699c6..d1f73e7734 100644 --- 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`. | 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) | diff --git a/projects/ngrx.io/content/guide/eslint-plugin/rules/avoid-combining-component-store-selectors.md b/projects/ngrx.io/content/guide/eslint-plugin/rules/avoid-combining-component-store-selectors.md new file mode 100644 index 0000000000..3116cab635 --- /dev/null +++ b/projects/ngrx.io/content/guide/eslint-plugin/rules/avoid-combining-component-store-selectors.md @@ -0,0 +1,92 @@ +# avoid-combining-component-store-selectors + +Prefer combining selectors at the selector level. + +- **Type**: suggestion +- **Recommended**: Yes +- **Fixable**: No +- **Suggestion**: No +- **Requires type checking**: No +- **Configurable**: No + + + + +## Rule Details + +Examples of **incorrect** code for this rule: + +### Enrich state with other state in component + +```ts +export class Component extends ComponentStore { + all$ = combineLatest( + this.select((state) => state.movies), + this.select((state) => state.books), + ); + + constructor() { + super({ movies: [], books: [] }) + } +} +``` + +### Filter state in component + +```ts +export class Component extends ComponentStore { + movie$ = combineLatest( + this.select((state) => state.movies), + this.select((state) => state.selectedId), + ).pipe( + map(([movies, selectedId]) => movies[selectedId]) + ); + + constructor() { + super({ movies: [] }) + } +} +``` + +Examples of **correct** code for this rule: + +### Enrich state with other state in selector + +```ts +export class Component extends ComponentStore { + movies$ = this.select((state) => state.movies); + books$ = this.select((state) => state.books); + all$ = this.select( + this.movies$, + this.books$, + ([movies, books]) => { + return { + movies, + books + } + } + ); + + constructor() { + super({ movies: [], books: [] }) + } +} +``` + +### Filter state in selector + +```ts +export class Component extends ComponentStore { + 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: [] }) + } +} +```