refactor: general refactor/fixes (#797)

* refactor: make use of 'isNotNullOrUndefined' and add tslint rule to enforce file name casing * fix: 'getDecoratorName' not handling decorators without parenthesis * refactor(utils): add missing decorators and normalize enum/constants/interfaces/types names to Angular* * refactor: order tslint.json * fix: add missing decorators for 'NgModule' * fix: parseInvalidSource method not handling other chars * fix: remove unnecessary export * refactor: improve testHelper * fix: adjust file-name-casing * refactor: change decorator constant name
mgechev · Apr 14, 2019 · 8d816c8 · 8d816c8
1 parent 6d283e6
commit 8d816c8
Show file tree

Hide file tree

Showing 29 changed files with 471 additions and 367 deletions.
diff --git a/src/angularWhitespaceRule.ts b/src/angularWhitespaceRule.ts
@@ -6,6 +6,7 @@ import { ExpTypes } from './angular/expressionTypes';
 import { NgWalker, NgWalkerConfig } from './angular/ngWalker';
 import { BasicTemplateAstVisitor } from './angular/templates/basicTemplateAstVisitor';
 import { RecursiveAngularExpressionVisitor } from './angular/templates/recursiveAngularExpressionVisitor';
+import { isNotNullOrUndefined } from './util/isNotNullOrUndefined';
 
 // Check if ES6 'y' flag is usable.
 const stickyFlagUsable = (() => {
@@ -173,7 +174,7 @@ class TemplateVisitorCtrl extends BasicTemplateAstVisitor {
     this.visitors
       .filter(v => options.indexOf(v.getCheckOption()) >= 0)
       .map(v => v.visitBoundText(text, this))
-      .filter(Boolean)
+      .filter(isNotNullOrUndefined)
       .forEach(f =>
         this.addFailureFromStartToEnd(f.getStartPosition().getPosition(), f.getEndPosition().getPosition(), f.getFailure(), f.getFix())
       );
@@ -185,7 +186,7 @@ class TemplateVisitorCtrl extends BasicTemplateAstVisitor {
     this.visitors
       .filter(v => options.indexOf(v.getCheckOption()) >= 0)
       .map(v => v.visitDirectiveProperty(prop, this))
-      .filter(Boolean)
+      .filter(isNotNullOrUndefined)
       .forEach(f =>
         this.addFailureFromStartToEnd(f.getStartPosition().getPosition(), f.getEndPosition().getPosition(), f.getFailure(), f.getFix())
       );
@@ -270,7 +271,7 @@ class ExpressionVisitorCtrl extends RecursiveAngularExpressionVisitor {
       .map(v => v.addParentAST(this.parentAST))
       .filter(v => options.indexOf(v.getCheckOption()) >= 0)
       .map(v => v.visitPipe(expr, this))
-      .filter(Boolean)
+      .filter(isNotNullOrUndefined)
       .forEach(f =>
         this.addFailureFromStartToEnd(f.getStartPosition().getPosition(), f.getEndPosition().getPosition(), f.getFailure(), f.getFix())
       );

diff --git a/src/componentSelectorRule.ts b/src/componentSelectorRule.ts
@@ -9,7 +9,7 @@ import {
   SelectorStyle,
   SelectorType
 } from './selectorPropertyBase';
-import { isNotNullOrUndefined } from './util/is-not-null-or-undefined';
+import { isNotNullOrUndefined } from './util/isNotNullOrUndefined';
 
 const STYLE_GUIDE_PREFIX_LINK = 'https://angular.io/guide/styleguide#style-02-07';
 const STYLE_GUIDE_STYLE_LINK = 'https://angular.io/guide/styleguide#style-05-02';

diff --git a/src/contextualDecoratorRule.ts b/src/contextualDecoratorRule.ts
@@ -1,36 +1,45 @@
 import { sprintf } from 'sprintf-js';
 import { IRuleMetadata, RuleFailure } from 'tslint';
 import { AbstractRule } from 'tslint/lib/rules';
+import { dedent } from 'tslint/lib/utils';
 import { createNodeArray, Decorator, isClassDeclaration, SourceFile } from 'typescript';
 import { NgWalker } from './angular/ngWalker';
 import {
-  DecoratorKeys,
-  Decorators,
+  ANGULAR_CLASS_DECORATOR_MAPPER,
+  AngularClassDecoratorKeys,
+  AngularClassDecorators,
+  AngularInnerClassDecoratorKeys,
+  AngularInnerClassDecorators,
   getDecoratorName,
   getNextToLastParentNode,
-  isMetadataType,
-  isNgDecorator,
-  METADATA_TYPE_DECORATOR_MAPPER,
-  MetadataTypes
+  isAngularClassDecorator,
+  isAngularInnerClassDecorator
 } from './util/utils';
 
 interface FailureParameters {
   readonly className: string;
-  readonly decoratorName: DecoratorKeys;
-  readonly metadataType: MetadataTypes;
+  readonly classDecoratorName: AngularClassDecoratorKeys;
+  readonly innerClassDecoratorName: AngularInnerClassDecoratorKeys;
 }
 
 export const getFailureMessage = (failureParameters: FailureParameters): string =>
-  sprintf(Rule.FAILURE_STRING, failureParameters.decoratorName, failureParameters.className, failureParameters.metadataType);
+  sprintf(
+    Rule.FAILURE_STRING,
+    failureParameters.innerClassDecoratorName,
+    failureParameters.className,
+    failureParameters.classDecoratorName
+  );
 
 export class Rule extends AbstractRule {
   static readonly metadata: IRuleMetadata = {
     description: 'Ensures that classes use allowed decorator in its body.',
     options: null,
     optionsDescription: 'Not configurable.',
-    rationale: `Some decorators can only be used in certain class types. For example, an @${Decorators.Input} should not be used in an @${
-      MetadataTypes.Injectable
-    } class.`,
+    rationale: dedent`
+      Some decorators can only be used in certain class types.
+      For example, an @${AngularInnerClassDecorators.Input} should not be used
+      in an @${AngularClassDecorators.Injectable} class.
+    `,
     ruleName: 'contextual-decorator',
     type: 'functionality',
     typescriptOnly: true
@@ -61,23 +70,23 @@ class Walker extends NgWalker {
 
     if (!isClassDeclaration(klass) || !klass.name) return;
 
-    const metadataType = createNodeArray(klass.decorators)
+    const classDecoratorName = createNodeArray(klass.decorators)
       .map(x => x.expression.getText())
       .map(x => x.replace(/[^a-zA-Z]/g, ''))
-      .find(isMetadataType);
+      .find(isAngularClassDecorator);
 
-    if (!metadataType) return;
+    if (!classDecoratorName) return;
 
-    const decoratorName = getDecoratorName(decorator);
+    const innerClassDecoratorName = getDecoratorName(decorator);
 
-    if (!decoratorName || !isNgDecorator(decoratorName)) return;
+    if (!innerClassDecoratorName || !isAngularInnerClassDecorator(innerClassDecoratorName)) return;
 
-    const allowedDecorators = METADATA_TYPE_DECORATOR_MAPPER[metadataType];
+    const allowedDecorators = ANGULAR_CLASS_DECORATOR_MAPPER.get(classDecoratorName);
 
-    if (!allowedDecorators || allowedDecorators.has(decoratorName)) return;
+    if (!allowedDecorators || allowedDecorators.has(innerClassDecoratorName)) return;
 
     const className = klass.name.getText();
-    const failure = getFailureMessage({ className, decoratorName, metadataType });
+    const failure = getFailureMessage({ classDecoratorName, className, innerClassDecoratorName });
 
     this.addFailureAtNode(decorator, failure);
   }

diff --git a/src/contextualLifecycleRule.ts b/src/contextualLifecycleRule.ts
@@ -1,38 +1,41 @@
 import { sprintf } from 'sprintf-js';
 import { IRuleMetadata, RuleFailure } from 'tslint/lib';
 import { AbstractRule } from 'tslint/lib/rules';
+import { dedent } from 'tslint/lib/utils';
 import { SourceFile } from 'typescript';
 import { InjectableMetadata, ModuleMetadata, PipeMetadata } from './angular';
 import { NgWalker } from './angular/ngWalker';
 import {
+  ANGULAR_CLASS_DECORATOR_LIFECYCLE_METHOD_MAPPER,
+  AngularClassDecoratorKeys,
+  AngularClassDecorators,
+  AngularLifecycleMethodKeys,
+  AngularLifecycleMethods,
   getClassName,
   getDecoratorName,
-  isLifecycleMethod,
-  isMetadataType,
-  LifecycleMethodKeys,
-  LifecycleMethods,
-  METADATA_TYPE_LIFECYCLE_MAPPER,
-  MetadataTypeKeys,
-  MetadataTypes
+  isAngularClassDecorator,
+  isAngularLifecycleMethod
 } from './util/utils';
 
 interface FailureParameters {
   readonly className: string;
-  readonly metadataType: MetadataTypeKeys;
-  readonly methodName: LifecycleMethodKeys;
+  readonly decoratorName: AngularClassDecoratorKeys;
+  readonly methodName: AngularLifecycleMethodKeys;
 }
 
 export const getFailureMessage = (failureParameters: FailureParameters): string =>
-  sprintf(Rule.FAILURE_STRING, failureParameters.methodName, failureParameters.className, failureParameters.metadataType);
+  sprintf(Rule.FAILURE_STRING, failureParameters.methodName, failureParameters.className, failureParameters.decoratorName);
 
 export class Rule extends AbstractRule {
   static readonly metadata: IRuleMetadata = {
     description: 'Ensures that classes use allowed lifecycle method in its body.',
     options: null,
     optionsDescription: 'Not configurable.',
-    rationale: `Some lifecycle methods can only be used in certain class types. For example, ${
-      LifecycleMethods.ngOnInit
-    }() method should not be used in an @${MetadataTypes.Injectable} class.`,
+    rationale: dedent`
+      Some lifecycle methods can only be used in certain class types.
+      For example, ${AngularLifecycleMethods.ngOnInit}() method should not be used
+      in an @${AngularClassDecorators.Injectable} class.
+    `,
     ruleName: 'contextual-lifecycle',
     type: 'functionality',
     typescriptOnly: true
@@ -49,26 +52,38 @@ export class Rule extends AbstractRule {
 
 class Walker extends NgWalker {
   protected visitNgInjectable(metadata: InjectableMetadata): void {
-    this.validateDecorator(metadata, METADATA_TYPE_LIFECYCLE_MAPPER.Injectable);
+    const injectableAllowedMethods = ANGULAR_CLASS_DECORATOR_LIFECYCLE_METHOD_MAPPER.get(AngularClassDecorators.Injectable);
+
+    if (!injectableAllowedMethods) return;
+
+    this.validateDecorator(metadata, injectableAllowedMethods);
     super.visitNgInjectable(metadata);
   }
 
-  protected visitNgPipe(metadata: PipeMetadata): void {
-    this.validateDecorator(metadata, METADATA_TYPE_LIFECYCLE_MAPPER.Pipe);
-    super.visitNgPipe(metadata);
-  }
+  protected visitNgModule(metadata: ModuleMetadata): void {
+    const ngModuleAllowedMethods = ANGULAR_CLASS_DECORATOR_LIFECYCLE_METHOD_MAPPER.get(AngularClassDecorators.NgModule);
+
+    if (!ngModuleAllowedMethods) return;
 
-  protected visitNgModule(metadata: ModuleMetadata) {
-    this.validateDecorator(metadata, METADATA_TYPE_LIFECYCLE_MAPPER.NgModule);
+    this.validateDecorator(metadata, ngModuleAllowedMethods);
     super.visitNgModule(metadata);
   }
 
-  private validateDecorator(metadata: PipeMetadata, allowedMethods: ReadonlySet<LifecycleMethodKeys>): void {
+  protected visitNgPipe(metadata: PipeMetadata): void {
+    const pipeAllowedMethods = ANGULAR_CLASS_DECORATOR_LIFECYCLE_METHOD_MAPPER.get(AngularClassDecorators.Pipe);
+
+    if (!pipeAllowedMethods) return;
+
+    this.validateDecorator(metadata, pipeAllowedMethods);
+    super.visitNgPipe(metadata);
+  }
+
+  private validateDecorator(metadata: PipeMetadata, allowedMethods: ReadonlySet<AngularLifecycleMethodKeys>): void {
     const className = getClassName(metadata.controller)!;
 
-    const metadataType = getDecoratorName(metadata.decorator);
+    const decoratorName = getDecoratorName(metadata.decorator);
 
-    if (!metadataType || !isMetadataType(metadataType)) return;
+    if (!decoratorName || !isAngularClassDecorator(decoratorName)) return;
 
     for (const member of metadata.controller.members) {
       const { name: memberName } = member;
@@ -77,9 +92,9 @@ class Walker extends NgWalker {
 
       const methodName = memberName.getText();
 
-      if (!isLifecycleMethod(methodName) || allowedMethods.has(methodName)) continue;
+      if (!isAngularLifecycleMethod(methodName) || allowedMethods.has(methodName)) continue;
 
-      const failure = getFailureMessage({ className, metadataType, methodName });
+      const failure = getFailureMessage({ className, decoratorName, methodName });
 
       this.addFailureAtNode(member, failure);
     }

diff --git a/src/directiveClassSuffixRule.ts b/src/directiveClassSuffixRule.ts
@@ -3,7 +3,7 @@ import * as Lint from 'tslint';
 import * as ts from 'typescript';
 import { DirectiveMetadata } from './angular/metadata';
 import { NgWalker } from './angular/ngWalker';
-import { getSymbolName } from './util/utils';
+import { getDeclaredInterfaceNames } from './util/utils';
 
 const ValidatorSuffix = 'Validator';
 
@@ -40,25 +40,17 @@ export class Rule extends Lint.Rules.AbstractRule {
 }
 
 class Walker extends NgWalker {
-  protected visitNgDirective(metadata: DirectiveMetadata) {
+  protected visitNgDirective(metadata: DirectiveMetadata): void {
     const name = metadata.controller.name!;
     const className = name.text;
     const options = this.getOptions();
     const suffixes: string[] = options.length ? options : ['Directive'];
-    const { heritageClauses } = metadata.controller;
 
-    if (heritageClauses) {
-      const i = heritageClauses.filter(h => h.token === ts.SyntaxKind.ImplementsKeyword);
+    const declaredInterfaceNames = getDeclaredInterfaceNames(metadata.controller);
+    const hasValidatorInterface = declaredInterfaceNames.some(interfaceName => interfaceName.endsWith(ValidatorSuffix));
 
-      if (
-        i.length !== 0 &&
-        i[0].types
-          .map(getSymbolName)
-          .filter(Boolean)
-          .some(x => x.endsWith(ValidatorSuffix))
-      ) {
-        suffixes.push(ValidatorSuffix);
-      }
+    if (hasValidatorInterface) {
+      suffixes.push(ValidatorSuffix);
     }
 
     if (!Rule.validate(className, suffixes)) {

diff --git a/src/directiveSelectorRule.ts b/src/directiveSelectorRule.ts
@@ -9,7 +9,7 @@ import {
   SelectorStyle,
   SelectorType
 } from './selectorPropertyBase';
-import { isNotNullOrUndefined } from './util/is-not-null-or-undefined';
+import { isNotNullOrUndefined } from './util/isNotNullOrUndefined';
 
 const STYLE_GUIDE_PREFIX_LINK = 'https://angular.io/guide/styleguide#style-02-08';
 const STYLE_GUIDE_STYLE_TYPE_LINK = 'https://angular.io/guide/styleguide#style-02-06';

diff --git a/src/noConflictingLifecycleRule.ts b/src/noConflictingLifecycleRule.ts
@@ -4,20 +4,26 @@ import { AbstractRule } from 'tslint/lib/rules';
 import { dedent } from 'tslint/lib/utils';
 import { ClassDeclaration, forEachChild, isClassDeclaration, Node, SourceFile } from 'typescript';
 import {
-  getDeclaredLifecycleInterfaces,
-  getDeclaredLifecycleMethods,
-  LifecycleInterfaceKeys,
-  LifecycleInterfaces,
-  LifecycleMethodKeys,
-  LifecycleMethods
+  AngularLifecycleInterfaceKeys,
+  AngularLifecycleInterfaces,
+  AngularLifecycleMethodKeys,
+  AngularLifecycleMethods,
+  getDeclaredAngularLifecycleInterfaces,
+  getDeclaredAngularLifecycleMethods
 } from './util/utils';
 
 interface FailureParameters {
   readonly message: typeof Rule.FAILURE_STRING_INTERFACE_HOOK | typeof Rule.FAILURE_STRING_METHOD_HOOK;
 }
 
-const LIFECYCLE_INTERFACES: ReadonlyArray<LifecycleInterfaceKeys> = [LifecycleInterfaces.DoCheck, LifecycleInterfaces.OnChanges];
-const LIFECYCLE_METHODS: ReadonlyArray<LifecycleMethodKeys> = [LifecycleMethods.ngDoCheck, LifecycleMethods.ngOnChanges];
+const LIFECYCLE_INTERFACES: ReadonlyArray<AngularLifecycleInterfaceKeys> = [
+  AngularLifecycleInterfaces.DoCheck,
+  AngularLifecycleInterfaces.OnChanges
+];
+const LIFECYCLE_METHODS: ReadonlyArray<AngularLifecycleMethodKeys> = [
+  AngularLifecycleMethods.ngDoCheck,
+  AngularLifecycleMethods.ngOnChanges
+];
 
 export const getFailureMessage = (failureParameters: FailureParameters): string => sprintf(failureParameters.message);
 
@@ -28,8 +34,8 @@ export class Rule extends AbstractRule {
     options: null,
     optionsDescription: 'Not configurable.',
     rationale: dedent`
-      A directive typically should not use both ${LifecycleInterfaces.DoCheck} and ${LifecycleInterfaces.OnChanges} to respond
-      to changes on the same input, as ${LifecycleMethods.ngOnChanges} will continue to be called when the
+      A directive typically should not use both ${AngularLifecycleInterfaces.DoCheck} and ${AngularLifecycleInterfaces.OnChanges} to respond
+      to changes on the same input, as ${AngularLifecycleMethods.ngOnChanges} will continue to be called when the
       default change detector detects changes.
     `,
     ruleName: 'no-conflicting-lifecycle',
@@ -38,10 +44,10 @@ export class Rule extends AbstractRule {
   };
 
   static readonly FAILURE_STRING_INTERFACE_HOOK = dedent`
-    Implementing ${LifecycleInterfaces.DoCheck} and ${LifecycleInterfaces.OnChanges} in a class is not recommended
+    Implementing ${AngularLifecycleInterfaces.DoCheck} and ${AngularLifecycleInterfaces.OnChanges} in a class is not recommended
   `;
   static readonly FAILURE_STRING_METHOD_HOOK = dedent`
-    Declaring ${LifecycleMethods.ngDoCheck} and ${LifecycleMethods.ngOnChanges} method in a class is not recommended
+    Declaring ${AngularLifecycleMethods.ngDoCheck} and ${AngularLifecycleMethods.ngOnChanges} method in a class is not recommended
   `;
 
   apply(sourceFile: SourceFile): RuleFailure[] {
@@ -55,9 +61,9 @@ const validateClassDeclaration = (context: WalkContext<void>, node: ClassDeclara
 };
 
 const validateInterfaces = (context: WalkContext<void>, node: ClassDeclaration): void => {
-  const declaredLifecycleInterfaces = getDeclaredLifecycleInterfaces(node);
-  const hasConflictingLifecycle = LIFECYCLE_INTERFACES.every(
-    lifecycleInterface => declaredLifecycleInterfaces.indexOf(lifecycleInterface) !== -1
+  const declaredAngularLifecycleInterfaces = getDeclaredAngularLifecycleInterfaces(node);
+  const hasConflictingLifecycle = LIFECYCLE_INTERFACES.every(lifecycleInterface =>
+    declaredAngularLifecycleInterfaces.includes(lifecycleInterface)
   );
 
   if (!hasConflictingLifecycle) return;
@@ -70,8 +76,8 @@ const validateInterfaces = (context: WalkContext<void>, node: ClassDeclaration):
 };
 
 const validateMethods = (context: WalkContext<void>, node: ClassDeclaration): void => {
-  const declaredLifecycleMethods = getDeclaredLifecycleMethods(node);
-  const hasConflictingLifecycle = LIFECYCLE_METHODS.every(lifecycleMethod => declaredLifecycleMethods.indexOf(lifecycleMethod) !== -1);
+  const declaredAngularLifecycleMethods = getDeclaredAngularLifecycleMethods(node);
+  const hasConflictingLifecycle = LIFECYCLE_METHODS.every(lifecycleMethod => declaredAngularLifecycleMethods.includes(lifecycleMethod));
 
   if (!hasConflictingLifecycle) return;