feat(eslint-plugin): add new rule require-super-ondestroy (#4611)

Closes #4505
ngrx · Dec 10, 2024 · 2ac4372 · 2ac4372
1 parent 4d34dc4
commit 2ac4372
Show file tree

Hide file tree

Showing 22 changed files with 267 additions and 48 deletions.
diff --git a/modules/eslint-plugin/schematics/ng-add/schema.ts b/modules/eslint-plugin/schematics/ng-add/schema.ts
@@ -1,4 +1,3 @@
-// eslint-disable-next-line @typescript-eslint/no-empty-interface
 export interface Schema {
   config: 'all' | 'store' | 'effects' | 'component-store' | 'signals';
 }
diff --git a/modules/eslint-plugin/spec/rules/component-store/require-super-ondestroy.spec.ts b/modules/eslint-plugin/spec/rules/component-store/require-super-ondestroy.spec.ts
@@ -0,0 +1,121 @@
+import { ESLintUtils } from '@typescript-eslint/utils';
+import { InvalidTestCase, ValidTestCase } from '@typescript-eslint/rule-tester';
+import rule, {
+  messageId,
+} from '../../../src/rules/component-store/require-super-ondestroy';
+import { fromFixture, ruleTester } from '../../utils';
+import path = require('path');
+
+type MessageIds = ESLintUtils.InferMessageIdsTypeFromRule<typeof rule>;
+type Options = ESLintUtils.InferOptionsTypeFromRule<typeof rule>;
+
+const valid: () => (string | ValidTestCase<Options>)[] = () => [
+  `
+import { ComponentStore } from '@ngrx/component-store';
+
+class BooksStore extends ComponentStore<BooksState>
+{
+}`,
+  `
+import { ComponentStore } from '@ngrx/component-store';
+
+class BooksStore extends ComponentStore<BooksState> implements OnDestroy
+{
+  override ngOnDestroy(): void {
+    super.ngOnDestroy();
+  }
+}`,
+  `
+import { ComponentStore } from '@ngrx/component-store';
+
+class BooksStore extends ComponentStore<BooksState> implements OnDestroy
+{
+  cleanUp() {}
+
+  override ngOnDestroy(): void {
+    this.cleanUp();
+    super.ngOnDestroy();
+  }
+}`,
+  `
+import { ComponentStore } from '@ngrx/component-store';
+
+class BooksStore extends ComponentStore<BooksState> implements OnDestroy
+{
+  cleanUp() {}
+
+  override ngOnDestroy(): void {
+    super.ngOnDestroy();
+    this.cleanUp();
+  }
+}`,
+  `
+import { ComponentStore } from '@ngrx/component-store';
+
+class BooksStore extends ComponentStore<BooksState> implements OnDestroy
+{
+  cleanUp() {}
+
+  override ngOnDestroy(): void {
+    this.cleanUp();
+    super.ngOnDestroy();
+    this.cleanUp();
+  }
+}`,
+  `
+import { ComponentStore } from '../components/component-store';
+
+class BooksStore extends ComponentStore implements OnDestroy
+{
+  cleanUp() {}
+
+  override ngOnDestroy(): void {
+    this.cleanUp();
+  }
+}`,
+];
+
+const invalid: () => InvalidTestCase<MessageIds, Options>[] = () => [
+  fromFixture(`
+import { ComponentStore } from '@ngrx/component-store';
+
+class BooksStore extends ComponentStore<BooksState> implements OnDestroy {
+  override ngOnDestroy(): void {
+           ~~~~~~~~~~~ [${messageId}]
+  }
+}`),
+  fromFixture(`
+import { ComponentStore } from '@ngrx/component-store';
+
+class BooksStore extends ComponentStore<BooksState> implements OnDestroy {
+  cleanUp() {}
+
+  override ngOnDestroy(): void {
+           ~~~~~~~~~~~ [${messageId}]
+    this.cleanUp();
+  }
+}`),
+  fromFixture(`
+import { ComponentStore } from '@ngrx/component-store';
+
+class BooksStore extends ComponentStore<BooksState> implements OnDestroy {
+  override ngOnDestroy(): void {
+           ~~~~~~~~~~~ [${messageId}]
+    super.ngOnDestroy;
+  }
+}`),
+  fromFixture(`
+import { ComponentStore } from '@ngrx/component-store';
+
+class BooksStore extends ComponentStore<BooksState> implements OnDestroy {
+  override ngOnDestroy(): void {
+           ~~~~~~~~~~~ [${messageId}]
+    super.get();
+  }
+}`),
+];
+
+ruleTester().run(path.parse(__filename).name, rule, {
+  valid: valid(),
+  invalid: invalid(),
+});
diff --git a/modules/eslint-plugin/src/configs/all.json b/modules/eslint-plugin/src/configs/all.json
@@ -4,6 +4,7 @@
   "rules": {
     "@ngrx/avoid-combining-component-store-selectors": "error",
     "@ngrx/avoid-mapping-component-store-selectors": "error",
+    "@ngrx/require-super-ondestroy": "error",
     "@ngrx/updater-explicit-return-type": "error",
     "@ngrx/avoid-cyclic-effects": "error",
     "@ngrx/no-dispatch-in-effects": "error",
@@ -13,9 +14,9 @@
     "@ngrx/prefer-effect-callback-in-block-statement": "error",
     "@ngrx/use-effects-lifecycle-interface": "error",
     "@ngrx/prefer-concat-latest-from": "error",
+    "@ngrx/prefer-protected-state": "error",
     "@ngrx/signal-state-no-arrays-at-root-level": "error",
     "@ngrx/signal-store-feature-should-use-generic-type": "error",
-    "@ngrx/prefer-protected-state": "error",
     "@ngrx/with-state-no-arrays-at-root-level": "error",
     "@ngrx/avoid-combining-selectors": "error",
     "@ngrx/avoid-dispatching-multiple-actions-sequentially": "error",

diff --git a/modules/eslint-plugin/src/configs/all.ts b/modules/eslint-plugin/src/configs/all.ts
@@ -32,6 +32,7 @@ export default (
     rules: {
       '@ngrx/avoid-combining-component-store-selectors': 'error',
       '@ngrx/avoid-mapping-component-store-selectors': 'error',
+      '@ngrx/require-super-ondestroy': 'error',
       '@ngrx/updater-explicit-return-type': 'error',
       '@ngrx/avoid-cyclic-effects': 'error',
       '@ngrx/no-dispatch-in-effects': 'error',
@@ -41,9 +42,9 @@ export default (
       '@ngrx/prefer-effect-callback-in-block-statement': 'error',
       '@ngrx/use-effects-lifecycle-interface': 'error',
       '@ngrx/prefer-concat-latest-from': 'error',
+      '@ngrx/prefer-protected-state': 'error',
       '@ngrx/signal-state-no-arrays-at-root-level': 'error',
       '@ngrx/signal-store-feature-should-use-generic-type': 'error',
-      '@ngrx/prefer-protected-state': 'error',
       '@ngrx/with-state-no-arrays-at-root-level': 'error',
       '@ngrx/avoid-combining-selectors': 'error',
       '@ngrx/avoid-dispatching-multiple-actions-sequentially': 'error',

diff --git a/modules/eslint-plugin/src/configs/component-store.json b/modules/eslint-plugin/src/configs/component-store.json
@@ -4,6 +4,7 @@
   "rules": {
     "@ngrx/avoid-combining-component-store-selectors": "error",
     "@ngrx/avoid-mapping-component-store-selectors": "error",
+    "@ngrx/require-super-ondestroy": "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
@@ -27,6 +27,7 @@ export default (
     rules: {
       '@ngrx/avoid-combining-component-store-selectors': 'error',
       '@ngrx/avoid-mapping-component-store-selectors': 'error',
+      '@ngrx/require-super-ondestroy': 'error',
       '@ngrx/updater-explicit-return-type': 'error',
     },
   },

diff --git a/modules/eslint-plugin/src/configs/signals.json b/modules/eslint-plugin/src/configs/signals.json
@@ -2,9 +2,9 @@
   "parser": "@typescript-eslint/parser",
   "plugins": ["@ngrx"],
   "rules": {
+    "@ngrx/prefer-protected-state": "error",
     "@ngrx/signal-state-no-arrays-at-root-level": "error",
     "@ngrx/signal-store-feature-should-use-generic-type": "error",
-    "@ngrx/prefer-protected-state": "error",
     "@ngrx/with-state-no-arrays-at-root-level": "error"
   },
   "parserOptions": {

diff --git a/modules/eslint-plugin/src/configs/signals.ts b/modules/eslint-plugin/src/configs/signals.ts
@@ -30,9 +30,9 @@ export default (
       },
     },
     rules: {
+      '@ngrx/prefer-protected-state': 'error',
       '@ngrx/signal-state-no-arrays-at-root-level': 'error',
       '@ngrx/signal-store-feature-should-use-generic-type': 'error',
-      '@ngrx/prefer-protected-state': 'error',
       '@ngrx/with-state-no-arrays-at-root-level': '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
@@ -1,11 +1,7 @@
 import type { TSESTree } from '@typescript-eslint/utils';
 import * as path from 'path';
 import { createRule } from '../../rule-creator';
-import {
-  asPattern,
-  getNgRxComponentStores,
-  namedExpression,
-} from '../../utils';
+import { getNgrxComponentStoreNames, namedExpression } from '../../utils';
 export const messageId = 'avoidCombiningComponentStoreSelectors';
 type MessageIds = typeof messageId;
 type Options = readonly [];
@@ -25,8 +21,7 @@ export default createRule<Options, MessageIds>({
   },
   defaultOptions: [],
   create: (context) => {
-    const { identifiers = [] } = getNgRxComponentStores(context);
-    const storeNames = identifiers.length > 0 ? asPattern(identifiers) : null;
+    const storeNames = getNgrxComponentStoreNames(context);
 
     const thisSelects = `CallExpression[callee.object.type='ThisExpression'][callee.property.name='select']`;
     const storeSelects = storeNames ? namedExpression(storeNames) : null;

diff --git a/modules/eslint-plugin/src/rules/component-store/avoid-mapping-component-store-selectors.ts b/modules/eslint-plugin/src/rules/component-store/avoid-mapping-component-store-selectors.ts
@@ -2,8 +2,7 @@ import type { TSESTree } from '@typescript-eslint/utils';
 import * as path from 'path';
 import { createRule } from '../../rule-creator';
 import {
-  asPattern,
-  getNgRxComponentStores,
+  getNgrxComponentStoreNames,
   namedCallableExpression,
 } from '../../utils';
 
@@ -27,8 +26,7 @@ export default createRule<Options, MessageIds>({
   },
   defaultOptions: [],
   create: (context) => {
-    const { identifiers = [] } = getNgRxComponentStores(context);
-    const storeNames = identifiers.length > 0 ? asPattern(identifiers) : null;
+    const storeNames = getNgrxComponentStoreNames(context);
 
     const mapOperatorSelector = `[callee.property.name=pipe] > CallExpression[callee.name=map]`;
     const selectors = [

diff --git a/modules/eslint-plugin/src/rules/component-store/require-super-ondestroy.ts b/modules/eslint-plugin/src/rules/component-store/require-super-ondestroy.ts
@@ -0,0 +1,51 @@
+import path = require('path');
+import { createRule } from '../../rule-creator';
+import { TSESTree } from '@typescript-eslint/types';
+
+export const messageId = 'requireSuperOnDestroy';
+type MessageIds = typeof messageId;
+type Options = readonly [];
+
+export default createRule<Options, MessageIds>({
+  name: path.parse(__filename).name,
+  meta: {
+    type: 'problem',
+    ngrxModule: 'component-store',
+    docs: {
+      description:
+        'Overriden ngOnDestroy method in component stores require a call to super.ngOnDestroy().',
+    },
+    schema: [],
+    messages: {
+      [messageId]:
+        "Call super.ngOnDestroy() inside a component store's ngOnDestroy method.",
+    },
+  },
+  defaultOptions: [],
+  create: (context) => {
+    const ngOnDestroyMethodSelector = `MethodDefinition[key.name='ngOnDestroy']`;
+    const componentStoreClassName = 'ComponentStore';
+
+    let hasNgrxComponentStoreImport = false;
+
+    return {
+      [`ImportDeclaration[source.value='@ngrx/component-store'] ImportSpecifier[imported.name='${componentStoreClassName}']`](
+        _: TSESTree.ImportSpecifier
+      ) {
+        hasNgrxComponentStoreImport = true;
+      },
+      [`ClassDeclaration[superClass.name=${componentStoreClassName}] ${ngOnDestroyMethodSelector}:not(:has(CallExpression[callee.object.type='Super'][callee.property.name='ngOnDestroy'])) > .key`](
+        node: TSESTree.Identifier
+      ) {
+        if (!hasNgrxComponentStoreImport) {
+          return;
+        }
+
+        context.report({
+          node,
+          messageId,
+        });
+      },
+    };
+  },
+});
diff --git a/modules/eslint-plugin/src/rules/component-store/updater-explicit-return-type.ts b/modules/eslint-plugin/src/rules/component-store/updater-explicit-return-type.ts
@@ -1,11 +1,7 @@
 import type { TSESTree } from '@typescript-eslint/utils';
 import * as path from 'path';
 import { createRule } from '../../rule-creator';
-import {
-  asPattern,
-  getNgRxComponentStores,
-  namedExpression,
-} from '../../utils';
+import { getNgrxComponentStoreNames, namedExpression } from '../../utils';
 
 export const messageId = 'updaterExplicitReturnType';
 
@@ -28,8 +24,7 @@ export default createRule<Options, MessageIds>({
   },
   defaultOptions: [],
   create: (context) => {
-    const { identifiers = [] } = getNgRxComponentStores(context);
-    const storeNames = identifiers.length > 0 ? asPattern(identifiers) : null;
+    const storeNames = getNgrxComponentStoreNames(context);
     const withoutTypeAnnotation = `ArrowFunctionExpression:not([returnType.typeAnnotation])`;
     const selectors = [
       `ClassDeclaration[superClass.name=/Store/] CallExpression[callee.object.type='ThisExpression'][callee.property.name='updater'] > ${withoutTypeAnnotation}`,

diff --git a/modules/eslint-plugin/src/rules/effects/no-effects-in-providers.ts b/modules/eslint-plugin/src/rules/effects/no-effects-in-providers.ts
@@ -31,7 +31,6 @@ export default createRule<Options, MessageIds>({
   },
   defaultOptions: [],
   create: (context) => {
-    const sourceCode = context.getSourceCode();
     const effectsInProviders = new Set<TSESTree.Identifier>();
     const effectsInImports = new Set<string>();
 
@@ -52,7 +51,11 @@ export default createRule<Options, MessageIds>({
             node: effectInProvider,
             messageId,
             fix: (fixer) =>
-              getNodeToCommaRemoveFix(sourceCode, fixer, effectInProvider),
+              getNodeToCommaRemoveFix(
+                context.sourceCode,
+                fixer,
+                effectInProvider
+              ),
           });
         }
 

diff --git a/modules/eslint-plugin/src/rules/effects/prefer-effect-callback-in-block-statement.ts b/modules/eslint-plugin/src/rules/effects/prefer-effect-callback-in-block-statement.ts
@@ -26,7 +26,6 @@ export default createRule<Options, MessageIds>({
   },
   defaultOptions: [],
   create: (context) => {
-    const sourceCode = context.getSourceCode();
     const nonParametrizedEffect =
       `${createEffectExpression} > ArrowFunctionExpression > .body[type!=/^(ArrowFunctionExpression|BlockStatement)$/]` as const;
     const parametrizedEffect =
@@ -43,7 +42,7 @@ export default createRule<Options, MessageIds>({
           messageId,
           fix: (fixer) => {
             const [previousNode, nextNode] = getSafeNodesToApplyFix(
-              sourceCode,
+              context.sourceCode,
               node
             );
             return [

diff --git a/modules/eslint-plugin/src/rules/index.ts b/modules/eslint-plugin/src/rules/index.ts
@@ -2,6 +2,7 @@
 import avoidCombiningComponentStoreSelectors from './component-store/avoid-combining-component-store-selectors';
 import avoidMappingComponentStoreSelectors from './component-store/avoid-mapping-component-store-selectors';
 import updaterExplicitReturnType from './component-store/updater-explicit-return-type';
+import requireSuperOnDestroy from './component-store/require-super-ondestroy';
 // effects
 import avoidCyclicEffects from './effects/avoid-cyclic-effects';
 import noDispatchInEffects from './effects/no-dispatch-in-effects';
@@ -44,6 +45,7 @@ export const rules = {
   'avoid-mapping-component-store-selectors':
     avoidMappingComponentStoreSelectors,
   'updater-explicit-return-type': updaterExplicitReturnType,
+  'require-super-ondestroy': requireSuperOnDestroy,
   //effects
   'avoid-cyclic-effects': avoidCyclicEffects,
   'no-dispatch-in-effects': noDispatchInEffects,

diff --git a/modules/eslint-plugin/src/rules/operators/prefer-concat-latest-from.ts b/modules/eslint-plugin/src/rules/operators/prefer-concat-latest-from.ts
@@ -61,7 +61,7 @@ export default createRule<Options, MessageIds>({
           context.report({
             node,
             messageId,
-            fix: (fixer) => getFixes(context.getSourceCode(), fixer, node),
+            fix: (fixer) => getFixes(context.sourceCode, fixer, node),
           });
         },
       };