diff --git a/README.md b/README.md index 98bb8a20..c5154b84 100644 --- a/README.md +++ b/README.md @@ -99,7 +99,7 @@ To enable the recommended configuration, add it to your ESLint configuration fil | [ngrx/prefer-inline-action-props](https://github.com/timdeschryver/eslint-plugin-ngrx/tree/main/docs/rules/prefer-inline-action-props.md) | Prefer using inline types instead of interfaces/classes. | suggestion | warn (Best Practices) | No | No | | [ngrx/prefer-one-generic-in-create-for-feature-selector](https://github.com/timdeschryver/eslint-plugin-ngrx/tree/main/docs/rules/prefer-one-generic-in-create-for-feature-selector.md) | Prefer using a single generic to define the feature state. | suggestion | warn (Best Practices) | No | No | | [ngrx/prefix-selectors-with-select](https://github.com/timdeschryver/eslint-plugin-ngrx/tree/main/docs/rules/prefix-selectors-with-select.md) | The selector should start with "select", for example "selectThing". | suggestion | warn (Best Practices) | No | No | -| [ngrx/select-style](https://github.com/timdeschryver/eslint-plugin-ngrx/tree/main/docs/rules/select-style.md) | Selectors can be used either with `select` as a pipeable operator or as a method. | problem | warn (Best Practices) | No | Yes | +| [ngrx/select-style](https://github.com/timdeschryver/eslint-plugin-ngrx/tree/main/docs/rules/select-style.md) | Selectors can be used either with `select` as a pipeable operator or as a method. | problem | warn (Best Practices) | Yes | Yes | | [ngrx/use-consistent-global-store-name](https://github.com/timdeschryver/eslint-plugin-ngrx/tree/main/docs/rules/use-consistent-global-store-name.md) | Use a consistent name for the global store. | suggestion | warn (Best Practices) | No | Yes | | [ngrx/use-selector-in-select](https://github.com/timdeschryver/eslint-plugin-ngrx/tree/main/docs/rules/use-selector-in-select.md) | Using a selector in a select method is preferred in favor of strings or props drilling. | suggestion | warn (Best Practices) | No | No | diff --git a/src/rules/effects/no-effect-decorator.ts b/src/rules/effects/no-effect-decorator.ts index 1dad521f..09687d8d 100644 --- a/src/rules/effects/no-effect-decorator.ts +++ b/src/rules/effects/no-effect-decorator.ts @@ -113,7 +113,7 @@ function getFixes( ].concat( getImportAddFix({ fixer, - importedName: createEffect, + importName: createEffect, moduleName: MODULE_PATHS.effects, node: classDeclaration, }), diff --git a/src/rules/effects/prefer-concat-latest-from.ts b/src/rules/effects/prefer-concat-latest-from.ts index 4d4b477a..b6be42bd 100644 --- a/src/rules/effects/prefer-concat-latest-from.ts +++ b/src/rules/effects/prefer-concat-latest-from.ts @@ -48,7 +48,7 @@ export default ESLintUtils.RuleCreator(docsUrl)({ return [fixer.replaceText(node, 'concatLatestFrom')].concat( getImportAddFix({ fixer, - importedName: 'concatLatestFrom', + importName: 'concatLatestFrom', moduleName: MODULE_PATHS.effects, node, }), diff --git a/src/rules/effects/use-effects-lifecycle-interface.ts b/src/rules/effects/use-effects-lifecycle-interface.ts index 79c308ea..80efde0a 100644 --- a/src/rules/effects/use-effects-lifecycle-interface.ts +++ b/src/rules/effects/use-effects-lifecycle-interface.ts @@ -70,7 +70,7 @@ export default ESLintUtils.RuleCreator(docsUrl)({ getImportAddFix({ compatibleWithTypeOnlyImport: true, fixer, - importedName: interfaceName, + importName: interfaceName, moduleName: MODULE_PATHS.effects, node: classDeclaration, }), diff --git a/src/rules/store/select-style.ts b/src/rules/store/select-style.ts index 76126021..ad6aeabc 100644 --- a/src/rules/store/select-style.ts +++ b/src/rules/store/select-style.ts @@ -1,9 +1,13 @@ -import type { TSESTree } from '@typescript-eslint/experimental-utils' +import type { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils' import { ESLintUtils } from '@typescript-eslint/experimental-utils' import path from 'path' import { docsUrl, findNgRxStoreName, + getImportAddFix, + getNearestUpperNodeFrom, + isClassDeclaration, + MODULE_PATHS, pipeableSelect, storeSelect, } from '../../utils' @@ -19,6 +23,20 @@ export const OPERATOR = 'operator' export const METHOD = 'method' type Options = [typeof OPERATOR | typeof METHOD] +type MemberExpressionWithProperty = Omit< + TSESTree.MemberExpression, + 'property' +> & { + property: TSESTree.Identifier +} +type CallExpression = Omit & { + callee: MemberExpressionWithProperty + parent: TSESTree.CallExpression & { + callee: Omit & { + object: MemberExpressionWithProperty + } + } +} export default ESLintUtils.RuleCreator(docsUrl)({ name: path.parse(__filename).name, @@ -30,6 +48,7 @@ export default ESLintUtils.RuleCreator(docsUrl)({ 'Selectors can be used either with `select` as a pipeable operator or as a method.', recommended: 'warn', }, + fixable: 'code', schema: [ { type: 'string', @@ -50,23 +69,88 @@ export default ESLintUtils.RuleCreator(docsUrl)({ if (!storeName) return {} if (mode === METHOD) { + const sourceCode = context.getSourceCode() + return { - [pipeableSelect(storeName)](node: TSESTree.CallExpression) { + [pipeableSelect(storeName)](node: CallExpression) { context.report({ - node, + node: node.callee, messageId: methodSelectMessageId, + fix: (fixer) => getOperatorToMethodFixes(node, sourceCode, fixer), }) }, } } return { - [storeSelect(storeName)](node: TSESTree.CallExpression) { + [storeSelect(storeName)](node: CallExpression) { context.report({ - node, + node: node.callee.property, messageId: operatorSelectMessageId, + fix: (fixer) => getMethodToOperatorFixes(node, fixer), }) }, } }, }) + +function getMethodToOperatorFixes( + node: CallExpression, + fixer: TSESLint.RuleFixer, +): TSESLint.RuleFix[] { + const classDeclaration = getNearestUpperNodeFrom(node, isClassDeclaration) + + if (!classDeclaration) { + return [] + } + + return [ + fixer.insertTextBefore(node.callee.property, 'pipe('), + fixer.insertTextAfter(node, ')'), + ].concat( + getImportAddFix({ + fixer, + importName: 'select', + moduleName: MODULE_PATHS.store, + node: classDeclaration, + }), + ) +} + +function getOperatorToMethodFixes( + node: CallExpression, + sourceCode: Readonly, + fixer: TSESLint.RuleFixer, +): TSESLint.RuleFix[] { + const { parent } = node + const pipeContainsOnlySelect = parent.arguments.length === 1 + + if (pipeContainsOnlySelect) { + const pipeRange: TSESTree.Range = [ + parent.callee.property.range[0], + parent.callee.property.range[1] + 1, + ] + const trailingParenthesisRange: TSESTree.Range = [ + parent.range[1] - 1, + parent.range[1], + ] + + return [ + fixer.removeRange(pipeRange), + fixer.removeRange(trailingParenthesisRange), + ] + } + + const text = sourceCode.getText(node) + const nextToken = sourceCode.getTokenAfter(node) + const selectOperatorRange: TSESTree.Range = [ + node.range[0], + nextToken?.range[1] ?? node.range[1], + ] + const storeRange = parent.callee.object.range + + return [ + fixer.removeRange(selectOperatorRange), + fixer.insertTextAfterRange(storeRange, `.${text}`), + ] +} diff --git a/src/utils/helper-functions/guards.ts b/src/utils/helper-functions/guards.ts index 66720b4c..d6523c1c 100644 --- a/src/utils/helper-functions/guards.ts +++ b/src/utils/helper-functions/guards.ts @@ -22,6 +22,12 @@ export const isIdentifier = isNodeOfType(AST_NODE_TYPES.Identifier) export const isImportDeclaration = isNodeOfType( AST_NODE_TYPES.ImportDeclaration, ) +export const isImportDefaultSpecifier = isNodeOfType( + AST_NODE_TYPES.ImportDefaultSpecifier, +) +export const isImportNamespaceSpecifier = isNodeOfType( + AST_NODE_TYPES.ImportNamespaceSpecifier, +) export const isImportSpecifier = isNodeOfType(AST_NODE_TYPES.ImportSpecifier) export const isLiteral = isNodeOfType(AST_NODE_TYPES.Literal) export const isMemberExpression = isNodeOfType(AST_NODE_TYPES.MemberExpression) @@ -31,6 +37,12 @@ export const isTSTypeReference = isNodeOfType(AST_NODE_TYPES.TSTypeReference) export const isObjectExpression = isNodeOfType(AST_NODE_TYPES.ObjectExpression) export const isProperty = isNodeOfType(AST_NODE_TYPES.Property) +export function isIdentifierOrMemberExpression( + node: TSESTree.Node, +): node is TSESTree.Identifier | TSESTree.MemberExpression { + return isIdentifier(node) || isMemberExpression(node) +} + export function isTypeReference(type: ts.Type): type is ts.TypeReference { return type.hasOwnProperty('target') } diff --git a/src/utils/helper-functions/utils.ts b/src/utils/helper-functions/utils.ts index fd348444..fc07811b 100644 --- a/src/utils/helper-functions/utils.ts +++ b/src/utils/helper-functions/utils.ts @@ -1,11 +1,12 @@ import type { TSESLint, TSESTree } from '@typescript-eslint/experimental-utils' -import { AST_NODE_TYPES } from '@typescript-eslint/experimental-utils' import { isCallExpression, isIdentifier, + isIdentifierOrMemberExpression, isImportDeclaration, + isImportDefaultSpecifier, + isImportNamespaceSpecifier, isImportSpecifier, - isMemberExpression, isProgram, isTSTypeAnnotation, isTSTypeReference, @@ -15,7 +16,7 @@ export const MODULE_PATHS = { componentStore: '@ngrx/component-store', effects: '@ngrx/effects', store: '@ngrx/store', -} +} as const export function getNearestUpperNodeFrom( { parent }: TSESTree.Node, @@ -34,14 +35,14 @@ export function getNearestUpperNodeFrom( export function getImportDeclarationSpecifier( importDeclarations: readonly TSESTree.ImportDeclaration[], - importedName: string, + importName: string, ) { for (const importDeclaration of importDeclarations) { const importSpecifier = importDeclaration.specifiers.find( (importClause): importClause is TSESTree.ImportSpecifier => { return ( isImportSpecifier(importClause) && - importClause.imported.name === importedName + importClause.imported.name === importName ) }, ) @@ -70,20 +71,42 @@ export function getImportDeclarations( ) } +function getCorrespondentImportClause( + importDeclarations: readonly TSESTree.ImportDeclaration[], + compatibleWithTypeOnlyImport = false, +) { + let importClause: TSESTree.ImportClause | undefined + + for (const { importKind, specifiers } of importDeclarations) { + const lastImportSpecifier = getLast(specifiers) + + if ( + (!compatibleWithTypeOnlyImport && importKind === 'type') || + isImportNamespaceSpecifier(lastImportSpecifier) + ) { + continue + } + + importClause = lastImportSpecifier + } + + return importClause +} + export function getImportAddFix({ compatibleWithTypeOnlyImport = false, fixer, - importedName, + importName, moduleName, node, }: { compatibleWithTypeOnlyImport?: boolean fixer: TSESLint.RuleFixer - importedName: string + importName: string moduleName: string node: TSESTree.Node }): TSESLint.RuleFix | TSESLint.RuleFix[] { - const fullImport = `import { ${importedName} } from '${moduleName}';\n` + const fullImport = `import { ${importName} } from '${moduleName}';\n` const importDeclarations = getImportDeclarations(node, moduleName) if (!importDeclarations?.length) { @@ -92,35 +115,26 @@ export function getImportAddFix({ const importDeclarationSpecifier = getImportDeclarationSpecifier( importDeclarations, - importedName, + importName, ) if (importDeclarationSpecifier) { return [] } - const [{ importKind, specifiers }] = importDeclarations + const importClause = getCorrespondentImportClause( + importDeclarations, + compatibleWithTypeOnlyImport, + ) - if (!compatibleWithTypeOnlyImport && importKind === 'type') { + if (!importClause) { return fixer.insertTextAfterRange([0, 0], fullImport) } - const lastImportSpecifier = getLast(specifiers) - - switch (lastImportSpecifier.type) { - case AST_NODE_TYPES.ImportDefaultSpecifier: - return fixer.insertTextAfter(lastImportSpecifier, `, { ${importedName} }`) - case AST_NODE_TYPES.ImportNamespaceSpecifier: - return fixer.insertTextAfterRange([0, 0], fullImport) - default: - return fixer.insertTextAfter(lastImportSpecifier, `, ${importedName}`) - } -} - -export function isIdentifierOrMemberExpression( - node: TSESTree.Node, -): node is TSESTree.Identifier | TSESTree.MemberExpression { - return isIdentifier(node) || isMemberExpression(node) + const replacementText = isImportDefaultSpecifier(importClause) + ? `, { ${importName} }` + : `, ${importName}` + return fixer.insertTextAfter(importClause, replacementText) } export function getInterfaceName( @@ -179,19 +193,19 @@ export function getDecoratorArgument({ expression }: TSESTree.Decorator) { function findCorrespondingNameBy( context: TSESLint.RuleContext, moduleName: string, - importedName: string, + importName: string, ): string | undefined { const { ast } = context.getSourceCode() const importDeclarations = getImportDeclarations(ast, moduleName) ?? [] const { importSpecifier } = - getImportDeclarationSpecifier(importDeclarations, importedName) ?? {} + getImportDeclarationSpecifier(importDeclarations, importName) ?? {} if (!importSpecifier) { return undefined } const variables = context.getDeclaredVariables(importSpecifier) - const typedVariable = variables.find(({ name }) => name === importedName) + const typedVariable = variables.find(({ name }) => name === importName) return typedVariable?.references .map(({ identifier: { parent } }) => { diff --git a/tests/rules/select-style.test.ts b/tests/rules/select-style.test.ts index 090d3567..78aa87fc 100644 --- a/tests/rules/select-style.test.ts +++ b/tests/rules/select-style.test.ts @@ -15,16 +15,16 @@ ruleTester().run(path.parse(__filename).name, rule, { import { Store } from '@ngrx/store' @Component() export class FixtureComponent { - foo$ = this.store.select(selector); - constructor(private store: Store){} + foo$ = this.store.select(selector) + constructor(private store: Store) {} } `, ` import { Store } from '@ngrx/store' @Component() export class FixtureComponent { - foo$ = this.customName.select(selector); - constructor(private customName: Store){} + foo$ = this.customName.select(selector) + constructor(private customName: Store) {} } `, { @@ -32,8 +32,8 @@ ruleTester().run(path.parse(__filename).name, rule, { import { Store } from '@ngrx/store' @Component() export class FixtureComponent { - foo$ = this.store.pipe(select(selector)); - constructor(private store: Store){} + foo$ = this.store.pipe(select(selector)) + constructor(private store: Store) {} } `, options: [OPERATOR], @@ -43,8 +43,8 @@ ruleTester().run(path.parse(__filename).name, rule, { import { Store } from '@ngrx/store' @Component() export class FixtureComponent { - foo$ = this.store.select(selector); - constructor(private store: Store){} + foo$ = this.store.select(selector) + constructor(private store: Store) {} } `, options: [METHOD], @@ -56,67 +56,114 @@ ruleTester().run(path.parse(__filename).name, rule, { import { select, Store } from '@ngrx/store' @Component() export class FixtureComponent { - foo$ = this.store.pipe(select(selector)); - ~~~~~~~~~~~~~~~~ [${methodSelectMessageId}] - - constructor(private store: Store){} + foo$ = this.store.pipe(select(selector)) + ~~~~~~ [${methodSelectMessageId}] + constructor(private store: Store) {} } `, + { + output: stripIndent` + import { select, Store } from '@ngrx/store' + @Component() + export class FixtureComponent { + foo$ = this.store.select(selector) + constructor(private store: Store) {} + } + `, + }, ), fromFixture( stripIndent` import { Store } from '@ngrx/store' @Component() export class FixtureComponent { - foo$ = this.store.pipe(select(selector)); - ~~~~~~~~~~~~~~~~ [${methodSelectMessageId}] - - constructor(private store: Store){} + foo$ = this.store.pipe(select(selector, selector2), filter(Boolean)) + ~~~~~~ [${methodSelectMessageId}] + constructor(private store: Store) {} } `, + { + options: [METHOD], + output: stripIndent` + import { Store } from '@ngrx/store' + @Component() + export class FixtureComponent { + foo$ = this.store.select(selector, selector2).pipe( filter(Boolean)) + constructor(private store: Store) {} + } + `, + }, ), fromFixture( stripIndent` - import { Store, select } from '@ngrx/store' + import { select, Store } from '@ngrx/store' @Component() export class FixtureComponent { - foo$; - - constructor(private store: Store){ - this.foo$ = store.pipe(select(selector)); - ~~~~~~~~~~~~~~~~ [${methodSelectMessageId}] + foo$ + constructor(private store: Store) { + this.foo$ = store.select(selector) + ~~~~~~ [${operatorSelectMessageId}] } } `, + { + options: [OPERATOR], + output: stripIndent` + import { select, Store } from '@ngrx/store' + @Component() + export class FixtureComponent { + foo$ + constructor(private store: Store) { + this.foo$ = store.pipe(select(selector)) + } + } + `, + }, ), fromFixture( stripIndent` import { Store } from '@ngrx/store' @Component() export class FixtureComponent { - foo$ = this.store.pipe(select(selector)); - ~~~~~~~~~~~~~~~~ [${methodSelectMessageId}] - - constructor(private store: Store){} + foo$ = this.store.pipe(select(selector), map(toItem)).pipe() + ~~~~~~ [${methodSelectMessageId}] + constructor(private store: Store) {} } `, { options: [METHOD], + output: stripIndent` + import { Store } from '@ngrx/store' + @Component() + export class FixtureComponent { + foo$ = this.store.select(selector).pipe( map(toItem)).pipe() + constructor(private store: Store) {} + } + `, }, ), fromFixture( stripIndent` + import type {Creator} from '@ngrx/store' import { Store } from '@ngrx/store' @Component() export class FixtureComponent { - foo$ = this.store.select(selector); - ~~~~~~~~~~~~~~~~~~~~~~~~~~~ [${operatorSelectMessageId}] - constructor(private store: Store){} + foo$ = this.store.select(selector) + ~~~~~~ [${operatorSelectMessageId}] + constructor(private store: Store) {} } - `, { options: [OPERATOR], + output: stripIndent` + import type {Creator} from '@ngrx/store' + import { Store, select } from '@ngrx/store' + @Component() + export class FixtureComponent { + foo$ = this.store.pipe(select(selector)) + constructor(private store: Store) {} + } + `, }, ), ],