From 66eebc2b594b26518d41641ae7dedb89275170ff Mon Sep 17 00:00:00 2001 From: Arya Khochare Date: Tue, 7 Jul 2026 01:23:51 +0530 Subject: [PATCH] Add ESLint rule to prevent Observable-returning method --- .eslintrc.json | 1 + docs/lint/html/index.md | 1 + .../rules/no-method-call-with-async-pipe.md | 161 ++++++++++++ lint/src/rules/html/index.ts | 2 + .../html/no-method-call-with-async-pipe.ts | 233 ++++++++++++++++++ 5 files changed, 398 insertions(+) create mode 100644 docs/lint/html/rules/no-method-call-with-async-pipe.md create mode 100644 lint/src/rules/html/no-method-call-with-async-pipe.ts diff --git a/.eslintrc.json b/.eslintrc.json index 9102c4719b7..bda42cef8e2 100644 --- a/.eslintrc.json +++ b/.eslintrc.json @@ -341,6 +341,7 @@ // Custom DSpace Angular rules "dspace-angular-html/themed-component-usages": "error", "dspace-angular-html/no-disabled-attribute-on-button": "error", + "dspace-angular-html/no-method-call-with-async-pipe": "error", "@angular-eslint/template/prefer-control-flow": "error" } }, diff --git a/docs/lint/html/index.md b/docs/lint/html/index.md index e134e1070f4..b915e4029b3 100644 --- a/docs/lint/html/index.md +++ b/docs/lint/html/index.md @@ -3,3 +3,4 @@ _______ - [`dspace-angular-html/themed-component-usages`](./rules/themed-component-usages.md): Themeable components should be used via the selector of their `ThemedComponent` wrapper class - [`dspace-angular-html/no-disabled-attribute-on-button`](./rules/no-disabled-attribute-on-button.md): Buttons should use the `dsBtnDisabled` directive instead of the HTML `disabled` attribute. +- [`dspace-angular-html/no-method-call-with-async-pipe`](./rules/no-method-call-with-async-pipe.md): Don't call a method directly as the input of the `async` pipe (e.g. `getFoo$() | async`). diff --git a/docs/lint/html/rules/no-method-call-with-async-pipe.md b/docs/lint/html/rules/no-method-call-with-async-pipe.md new file mode 100644 index 00000000000..008bca34ae3 --- /dev/null +++ b/docs/lint/html/rules/no-method-call-with-async-pipe.md @@ -0,0 +1,161 @@ +[DSpace ESLint plugins](../../../../lint/README.md) > [HTML rules](../index.md) > `dspace-angular-html/no-method-call-with-async-pipe` +_______ + +Don't call a method directly as the input of the `async` pipe (e.g. `getFoo$() | async`). + +This re-executes the method - and the RxJS pipeline it returns - on every change detection cycle, which often leads to performance issues. + +Instead: +- Subscribe to the Observable in `ngOnInit()` +- Push emitted values into a `BehaviorSubject` +- Bind `| async` to that `BehaviorSubject` in the template +- Unsubscribe in `ngOnDestroy()` + + +_______ + +[Source code](../../../../lint/src/rules/html/no-method-call-with-async-pipe.ts) + + + +### Examples + + +#### Valid code + +##### binding the async pipe to a property is still valid + +```html +{{ foo$ | async }} +``` + + +##### calling a method without piping the result through async is still valid + +```html +{{ getFoo() }} +``` + + +##### piping a property through an unrelated pipe is still valid + +```html +{{ foo$ | someOtherPipe }} +``` + + +##### binding the async pipe to a property accessed through a member expression is still valid + +```html +{{ (foo$ | async)?.length }} +``` + + + + + +#### Invalid code + +##### should not call a method as the direct input of the async pipe + +```html +{{ getFoo$() | async }} + + + +``` +Will produce the following error(s): +``` +Don't call '{{ methodName }}' directly in the template as the input of the async pipe; subscribe in ngOnInit(), push values into a BehaviorSubject, and bind `| async` to that instead. +``` + + +##### should not call a method with arguments as the direct input of the async pipe + +```html +{{ getFoo$(id) | async }} + + + +``` +Will produce the following error(s): +``` +Don't call '{{ methodName }}' directly in the template as the input of the async pipe; subscribe in ngOnInit(), push values into a BehaviorSubject, and bind `| async` to that instead. +``` + + +##### should not use the safe-call variant as the direct input of the async pipe + +```html +{{ getFoo$?.() | async }} + + + +``` +Will produce the following error(s): +``` +Don't call '{{ methodName }}' directly in the template as the input of the async pipe; subscribe in ngOnInit(), push values into a BehaviorSubject, and bind `| async` to that instead. +``` + + +##### should not call a method as the direct input of the async pipe in the *ngIf microsyntax + +```html +
{{ foo }}
+ + + +``` +Will produce the following error(s): +``` +Don't call '{{ methodName }}' directly in the template as the input of the async pipe; subscribe in ngOnInit(), push values into a BehaviorSubject, and bind `| async` to that instead. +``` + + +##### should not call a method as the direct input of the async pipe in an @if condition, even when wrapped in a member access + +```html +@if ((getFoo$() | async)?.length > 0) { + hi +} + + + +``` +Will produce the following error(s): +``` +Don't call '{{ methodName }}' directly in the template as the input of the async pipe; subscribe in ngOnInit(), push values into a BehaviorSubject, and bind `| async` to that instead. +``` + + +##### should not call a method as the direct input of the async pipe in an @for expression + +```html +@for (item of getFoo$() | async; track item) { + {{ item }} +} + + + +``` +Will produce the following error(s): +``` +Don't call '{{ methodName }}' directly in the template as the input of the async pipe; subscribe in ngOnInit(), push values into a BehaviorSubject, and bind `| async` to that instead. +``` + + +##### should not call a method as the direct input of the async pipe inside an object literal binding + +```html +
+ + + +``` +Will produce the following error(s): +``` +Don't call '{{ methodName }}' directly in the template as the input of the async pipe; subscribe in ngOnInit(), push values into a BehaviorSubject, and bind `| async` to that instead. +``` + + + diff --git a/lint/src/rules/html/index.ts b/lint/src/rules/html/index.ts index 7c1da7fef28..8fdaeb7c622 100644 --- a/lint/src/rules/html/index.ts +++ b/lint/src/rules/html/index.ts @@ -11,11 +11,13 @@ import { RuleExports, } from '../../util/structure'; import * as noDisabledAttributeOnButton from './no-disabled-attribute-on-button'; +import * as noMethodCallWithAsyncPipe from './no-method-call-with-async-pipe'; import * as themedComponentUsages from './themed-component-usages'; const index = [ themedComponentUsages, noDisabledAttributeOnButton, + noMethodCallWithAsyncPipe, ] as unknown as RuleExports[]; diff --git a/lint/src/rules/html/no-method-call-with-async-pipe.ts b/lint/src/rules/html/no-method-call-with-async-pipe.ts new file mode 100644 index 00000000000..842ed872e28 --- /dev/null +++ b/lint/src/rules/html/no-method-call-with-async-pipe.ts @@ -0,0 +1,233 @@ +/** + * The contents of this file are subject to the license and copyright + * detailed in the LICENSE and NOTICE files at the root of the source + * tree and available online at + * + * http://www.dspace.org/license/ + */ +import { + AST, + BindingPipe, + Call, + PropertyRead, + RecursiveAstVisitor, + SafeCall, + TmplAstBoundAttribute, + TmplAstBoundDeferredTrigger, + TmplAstBoundEvent, + TmplAstBoundText, + TmplAstForLoopBlock, + TmplAstIfBlockBranch, + TmplAstLetDeclaration, + TmplAstSwitchBlock, + TmplAstSwitchBlockCase, +} from '@angular-eslint/bundled-angular-compiler'; +import { ESLintUtils } from '@typescript-eslint/utils'; +import { + RuleContext, + RuleListener, +} from '@typescript-eslint/utils/ts-eslint'; + +import { + DSpaceESLintRuleInfo, + NamedTests, +} from '../../util/structure'; +import { getSourceCode } from '../../util/typescript'; + +export enum Message { + NO_METHOD_CALL_WITH_ASYNC_PIPE = 'noMethodCallWithAsyncPipe', +} + +export const info = { + name: 'no-method-call-with-async-pipe', + meta: { + docs: { + description: `Don't call a method directly as the input of the \`async\` pipe (e.g. \`getFoo$() | async\`). + +This re-executes the method - and the RxJS pipeline it returns - on every change detection cycle, which often leads to performance issues. + +Instead: +- Subscribe to the Observable in \`ngOnInit()\` +- Push emitted values into a \`BehaviorSubject\` +- Bind \`| async\` to that \`BehaviorSubject\` in the template +- Unsubscribe in \`ngOnDestroy()\` + `, + }, + type: 'problem', + schema: [], + messages: { + [Message.NO_METHOD_CALL_WITH_ASYNC_PIPE]: 'Don\'t call \'{{ methodName }}\' directly in the template as the input of the async pipe; subscribe in ngOnInit(), push values into a BehaviorSubject, and bind `| async` to that instead.', + }, + }, + optionDocs: [], + defaultOptions: [], +} as DSpaceESLintRuleInfo; + +/** + * @angular-eslint/template-parser's own visitor keys (used to drive ESLint's node traversal) + * don't cover every expression AST node type (e.g. SafePropertyRead, LiteralMap, KeyedRead, ...), + * so selectors like `BindingPipe[name="async"] > Call` silently miss anything wrapped in one of + * those unsupported node types (e.g. `(getFoo$() | async)?.length`, or `[ngClass]="{x: getFoo$() | async}"`). + * We work around this by walking each expression ourselves with the Angular compiler's own + * RecursiveAstVisitor, which correctly covers the full expression AST regardless of what + * @angular-eslint/template-parser exposes to ESLint's selector engine. + */ +class AsyncPipedCallVisitor extends RecursiveAstVisitor { + constructor(private readonly onFound: (call: Call | SafeCall) => void) { + super(); + } + + override visitPipe(ast: BindingPipe, context: unknown) { + if (ast.name === 'async' && (ast.exp instanceof Call || ast.exp instanceof SafeCall)) { + this.onFound(ast.exp); + } + super.visitPipe(ast, context); + } +} + +export const rule = ESLintUtils.RuleCreator.withoutDocs({ + meta: info.meta, + defaultOptions: info.defaultOptions, + create(context: RuleContext): RuleListener { + const sourceCode = getSourceCode(context); + + const visitor = new AsyncPipedCallVisitor((node) => { + const methodName = node.receiver instanceof PropertyRead ? node.receiver.name : 'this method'; + + context.report({ + messageId: Message.NO_METHOD_CALL_WITH_ASYNC_PIPE, + loc: { + start: sourceCode.getLocFromIndex(node.sourceSpan.start), + end: sourceCode.getLocFromIndex(node.sourceSpan.end), + }, + data: { + methodName, + }, + }); + }); + + function checkExpression(expression: AST | null | undefined) { + if (expression != null) { + visitor.visit(expression); + } + } + + return { + BoundAttribute(node: TmplAstBoundAttribute) { + checkExpression(node.value); + }, + BoundEvent(node: TmplAstBoundEvent) { + checkExpression(node.handler); + }, + BoundText(node: TmplAstBoundText) { + checkExpression(node.value); + }, + IfBlockBranch(node: TmplAstIfBlockBranch) { + checkExpression(node.expression); + }, + ForLoopBlock(node: TmplAstForLoopBlock) { + checkExpression(node.expression); + checkExpression(node.trackBy); + }, + SwitchBlock(node: TmplAstSwitchBlock) { + checkExpression(node.expression); + }, + SwitchBlockCase(node: TmplAstSwitchBlockCase) { + checkExpression(node.expression); + }, + LetDeclaration(node: TmplAstLetDeclaration) { + checkExpression(node.value); + }, + BoundDeferredTrigger(node: TmplAstBoundDeferredTrigger) { + checkExpression(node.value); + }, + }; + }, +}); + +export const tests = { + plugin: info.name, + valid: [ + { + name: 'binding the async pipe to a property is still valid', + code: ` +{{ foo$ | async }} + `, + }, + { + name: 'calling a method without piping the result through async is still valid', + code: ` +{{ getFoo() }} + `, + }, + { + name: 'piping a property through an unrelated pipe is still valid', + code: ` +{{ foo$ | someOtherPipe }} + `, + }, + { + name: 'binding the async pipe to a property accessed through a member expression is still valid', + code: ` +{{ (foo$ | async)?.length }} + `, + }, + ], + invalid: [ + { + name: 'should not call a method as the direct input of the async pipe', + code: ` +{{ getFoo$() | async }} + `, + errors: [{ messageId: Message.NO_METHOD_CALL_WITH_ASYNC_PIPE }], + }, + { + name: 'should not call a method with arguments as the direct input of the async pipe', + code: ` +{{ getFoo$(id) | async }} + `, + errors: [{ messageId: Message.NO_METHOD_CALL_WITH_ASYNC_PIPE }], + }, + { + name: 'should not use the safe-call variant as the direct input of the async pipe', + code: ` +{{ getFoo$?.() | async }} + `, + errors: [{ messageId: Message.NO_METHOD_CALL_WITH_ASYNC_PIPE }], + }, + { + name: 'should not call a method as the direct input of the async pipe in the *ngIf microsyntax', + code: ` +
{{ foo }}
+ `, + errors: [{ messageId: Message.NO_METHOD_CALL_WITH_ASYNC_PIPE }], + }, + { + name: 'should not call a method as the direct input of the async pipe in an @if condition, even when wrapped in a member access', + code: ` +@if ((getFoo$() | async)?.length > 0) { + hi +} + `, + errors: [{ messageId: Message.NO_METHOD_CALL_WITH_ASYNC_PIPE }], + }, + { + name: 'should not call a method as the direct input of the async pipe in an @for expression', + code: ` +@for (item of getFoo$() | async; track item) { + {{ item }} +} + `, + errors: [{ messageId: Message.NO_METHOD_CALL_WITH_ASYNC_PIPE }], + }, + { + name: 'should not call a method as the direct input of the async pipe inside an object literal binding', + code: ` +
+ `, + errors: [{ messageId: Message.NO_METHOD_CALL_WITH_ASYNC_PIPE }], + }, + ], +} as NamedTests; + +export default rule;