Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Dec 10, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
216d7d7
feat(eslint-plugin): add new rule require-super-ondestroy #4505
markoblagdan Nov 27, 2024
0726d1c
feat(eslint-plugin): add new rule require-super-ondestroy
markoblagdan Nov 27, 2024
bcbc64b
feat(eslint-plugin): add new rule require-super-ondestroy
markoblagdan Nov 30, 2024
b31952c
feat(eslint-plugin): add new rule require-super-ondestroy
markoblagdan Nov 30, 2024
2c1fd97
feat(eslint-plugin): add new rule require-super-ondestroy
markoblagdan Dec 1, 2024
c9379a3
feat(eslint-plugin): add new rule require-super-ondestroy
markoblagdan Dec 8, 2024
c0fb1d3
feat(eslint-plugin): add new rule require-super-ondestroy
markoblagdan Dec 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion modules/eslint-plugin/schematics/ng-add/schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
// eslint-disable-next-line @typescript-eslint/no-empty-interface
export interface Schema {
config: 'all' | 'store' | 'effects' | 'component-store' | 'signals';
}
121 changes: 121 additions & 0 deletions modules/eslint-plugin/spec/rules/component-store/require-super-ondestroy.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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>)[] = () => [
timdeschryver marked this conversation as resolved.
Show resolved Hide resolved
`
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(),
});
3 changes: 2 additions & 1 deletion modules/eslint-plugin/src/configs/all.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down
3 changes: 2 additions & 1 deletion modules/eslint-plugin/src/configs/all.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand All @@ -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',
Expand Down
1 change: 1 addition & 0 deletions modules/eslint-plugin/src/configs/component-store.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
}
1 change: 1 addition & 0 deletions modules/eslint-plugin/src/configs/component-store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
},
},
Expand Down
2 changes: 1 addition & 1 deletion modules/eslint-plugin/src/configs/signals.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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": {
Expand Down
2 changes: 1 addition & 1 deletion modules/eslint-plugin/src/configs/signals.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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',
},
},
Expand Down
9 changes: 2 additions & 7 deletions modules/eslint-plugin/src/rules/component-store/avoid-combining-component-store-selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 [];
Expand All @@ -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;
Expand Down
6 changes: 2 additions & 4 deletions modules/eslint-plugin/src/rules/component-store/avoid-mapping-component-store-selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';

Expand All @@ -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 = [
Expand Down
51 changes: 51 additions & 0 deletions modules/eslint-plugin/src/rules/component-store/require-super-ondestroy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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`](
Copy link
Contributor

@amakhrov amakhrov Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this uses :has - which is technically supported by the underlying esquery, but is not officially supported by eslint. It's not mentioned in the docs here, and also see this issue: eslint/eslint#14076 (comment).

do you think this approach is reasonably future-proof?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was not aware of this - doesn't seem like support for it would be dropped, but maybe it's safer to use another approach.

@timdeschryver If I wanted to make a PR with an alternative to using :has, what would be the best way - should we create a new issue and I make a bugfix PR referencing it, or some other way?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to keep it as-is.
There are multiple rules using :has, and I like it because it's easy to read.
If it does become deprecated/removed in the future we can update it.

node: TSESTree.Identifier
) {
if (!hasNgrxComponentStoreImport) {
return;
}

context.report({
node,
messageId,
});
},
};
},
});
9 changes: 2 additions & 7 deletions modules/eslint-plugin/src/rules/component-store/updater-explicit-return-type.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';

Expand All @@ -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}`,
Expand Down
7 changes: 5 additions & 2 deletions modules/eslint-plugin/src/rules/effects/no-effects-in-providers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>();

Expand All @@ -52,7 +51,11 @@ export default createRule<Options, MessageIds>({
node: effectInProvider,
messageId,
fix: (fixer) =>
getNodeToCommaRemoveFix(sourceCode, fixer, effectInProvider),
getNodeToCommaRemoveFix(
context.sourceCode,
fixer,
effectInProvider
),
});
}

Expand Down
3 changes: 1 addition & 2 deletions modules/eslint-plugin/src/rules/effects/prefer-effect-callback-in-block-statement.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 =
Expand All @@ -43,7 +42,7 @@ export default createRule<Options, MessageIds>({
messageId,
fix: (fixer) => {
const [previousNode, nextNode] = getSafeNodesToApplyFix(
sourceCode,
context.sourceCode,
node
);
return [
Expand Down
2 changes: 2 additions & 0 deletions modules/eslint-plugin/src/rules/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion modules/eslint-plugin/src/rules/operators/prefer-concat-latest-from.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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),
});
},
};
Expand Down
Loading
Loading