From a2dbf0b52a38626a2f6c69d5cb1b8135f3f09412 Mon Sep 17 00:00:00 2001 From: WilliamKoza Date: Sat, 9 Mar 2019 21:03:53 +0100 Subject: [PATCH 1/3] improve ngWalker by preventing an error when a class has no name. --- src/angular/metadata.ts | 17 ++++++++ src/angular/metadataReader.ts | 65 ++++++++++++++++++++++++++-- src/angular/ngWalker.ts | 55 ++++++++++++----------- src/contextualLifecycleRule.ts | 17 ++++---- src/noPipeImpureRule.ts | 15 +++---- src/pipePrefixRule.ts | 9 ++-- test/angular/metadataReader.spec.ts | 67 ++++++++++++++++++----------- test/angular/ngWalker.spec.ts | 41 ++++++++++++++++++ 8 files changed, 210 insertions(+), 76 deletions(-) diff --git a/src/angular/metadata.ts b/src/angular/metadata.ts index 895fa45eb..4b0a86f5d 100644 --- a/src/angular/metadata.ts +++ b/src/angular/metadata.ts @@ -44,3 +44,20 @@ export class ComponentMetadata extends DirectiveMetadata { super(controller, decorator, selector); } } + +export class PipeMetadata { + constructor( + public readonly controller: ts.ClassDeclaration, + public readonly decorator: ts.Decorator, + public readonly name?: string, + public readonly pure?: string + ) {} +} + +export class ModuleMetadata { + constructor(public readonly controller: ts.ClassDeclaration, public readonly decorator: ts.Decorator) {} +} + +export class InjectableMetadata { + constructor(public readonly controller: ts.ClassDeclaration, public readonly decorator: ts.Decorator) {} +} diff --git a/src/angular/metadataReader.ts b/src/angular/metadataReader.ts index f874f15b4..1555beebd 100644 --- a/src/angular/metadataReader.ts +++ b/src/angular/metadataReader.ts @@ -1,12 +1,22 @@ import * as ts from 'typescript'; -import { callExpression, decoratorArgument, hasProperties, withIdentifier } from '../util/astQuery'; +import { callExpression, decoratorArgument, getStringInitializerFromProperty, hasProperties, withIdentifier } from '../util/astQuery'; import { ifTrue, listToMaybe, Maybe, unwrapFirst } from '../util/function'; import { logger } from '../util/logger'; import { getAnimations, getInlineStyle, getTemplate } from '../util/ngQuery'; import { getDecoratorPropertyInitializer, isStringLiteralLike, maybeNodeArray } from '../util/utils'; import { Config } from './config'; import { FileResolver } from './fileResolver/fileResolver'; -import { AnimationMetadata, CodeWithSourceMap, ComponentMetadata, DirectiveMetadata, StyleMetadata, TemplateMetadata } from './metadata'; +import { + AnimationMetadata, + CodeWithSourceMap, + ComponentMetadata, + DirectiveMetadata, + InjectableMetadata, + ModuleMetadata, + PipeMetadata, + StyleMetadata, + TemplateMetadata +} from './metadata'; import { AbstractResolver, MetadataUrls } from './urlResolvers/abstractResolver'; import { PathResolver } from './urlResolvers/pathResolver'; import { UrlResolver } from './urlResolvers/urlResolver'; @@ -26,7 +36,7 @@ export class MetadataReader { this.urlResolver = this.urlResolver || new UrlResolver(new PathResolver()); } - read(d: ts.ClassDeclaration): DirectiveMetadata | undefined { + read(d: ts.ClassDeclaration): DirectiveMetadata | ComponentMetadata | PipeMetadata | ModuleMetadata | InjectableMetadata | undefined { const componentMetadata = unwrapFirst( maybeNodeArray(d.decorators!).map(dec => { return Maybe.lift(dec) @@ -45,7 +55,34 @@ export class MetadataReader { ) ); - return directiveMetadata || componentMetadata || undefined; + const pipeMetadata = unwrapFirst( + maybeNodeArray(d.decorators!).map(dec => + Maybe.lift(dec) + .bind(callExpression) + .bind(withIdentifier('Pipe') as any) + .fmap(() => this.readPipeMetadata(d, dec)) + ) + ); + + const moduleMetadata = unwrapFirst( + maybeNodeArray(d.decorators!).map(dec => + Maybe.lift(dec) + .bind(callExpression) + .bind(withIdentifier('NgModule') as any) + .fmap(() => this.readModuleMetadata(d, dec)) + ) + ); + + const injectableMetadata = unwrapFirst( + maybeNodeArray(d.decorators!).map(dec => + Maybe.lift(dec) + .bind(callExpression) + .bind(withIdentifier('Injectable') as any) + .fmap(() => this.readInjectableMetadata(d, dec)) + ) + ); + + return directiveMetadata || componentMetadata || pipeMetadata || moduleMetadata || injectableMetadata || undefined; } protected readDirectiveMetadata(d: ts.ClassDeclaration, dec: ts.Decorator): DirectiveMetadata { @@ -55,6 +92,26 @@ export class MetadataReader { return new DirectiveMetadata(d, dec, selector); } + protected readPipeMetadata(d: ts.ClassDeclaration, dec: ts.Decorator): DirectiveMetadata { + const name = this.getDecoratorArgument(dec) + .bind(expr => getStringInitializerFromProperty('name', expr!.properties)) + .fmap(initializer => initializer!.text); + + const pure = this.getDecoratorArgument(dec) + .bind(expr => getStringInitializerFromProperty('pure', expr!.properties)) + .fmap(initializer => initializer!.text); + + return new PipeMetadata(d, dec, name.unwrap(), pure ? pure.unwrap() : undefined); + } + + protected readModuleMetadata(d: ts.ClassDeclaration, dec: ts.Decorator): DirectiveMetadata { + return new ModuleMetadata(d, dec); + } + + protected readInjectableMetadata(d: ts.ClassDeclaration, dec: ts.Decorator): DirectiveMetadata { + return new InjectableMetadata(d, dec); + } + protected readComponentMetadata(d: ts.ClassDeclaration, dec: ts.Decorator): ComponentMetadata { const expr = this.getDecoratorArgument(dec); const directiveMetadata = this.readDirectiveMetadata(d, dec); diff --git a/src/angular/ngWalker.ts b/src/angular/ngWalker.ts index bece9deb4..114dafcfe 100644 --- a/src/angular/ngWalker.ts +++ b/src/angular/ngWalker.ts @@ -2,9 +2,9 @@ import * as compiler from '@angular/compiler'; import * as Lint from 'tslint'; import * as ts from 'typescript'; import { logger } from '../util/logger'; -import { getDecoratorName, maybeNodeArray } from '../util/utils'; +import { getClassName, getDecoratorName, maybeNodeArray } from '../util/utils'; import { Config } from './config'; -import { ComponentMetadata, DirectiveMetadata, StyleMetadata } from './metadata'; +import { ComponentMetadata, DirectiveMetadata, InjectableMetadata, ModuleMetadata, PipeMetadata, StyleMetadata } from './metadata'; import { MetadataReader } from './metadataReader'; import { ngWalkerFactoryUtils } from './ngWalkerFactoryUtils'; import { BasicCssAstVisitor, CssAstVisitorCtrl } from './styles/basicCssAstVisitor'; @@ -61,11 +61,19 @@ export class NgWalker extends Lint.RuleWalker { } visitClassDeclaration(declaration: ts.ClassDeclaration) { - const metadata = this._metadataReader!.read(declaration); - if (metadata instanceof ComponentMetadata) { - this.visitNgComponent(metadata); - } else if (metadata instanceof DirectiveMetadata) { - this.visitNgDirective(metadata); + if (this.hasClassName(declaration)) { + const metadata = this._metadataReader!.read(declaration); + if (metadata instanceof ComponentMetadata) { + this.visitNgComponent(metadata); + } else if (metadata instanceof DirectiveMetadata) { + this.visitNgDirective(metadata); + } else if (metadata instanceof PipeMetadata) { + this.visitNgPipe(metadata); + } else if (metadata instanceof ModuleMetadata) { + this.visitNgModule(metadata); + } else if (metadata instanceof InjectableMetadata) { + this.visitNgInjectable(metadata); + } } maybeNodeArray(ts.createNodeArray(declaration.decorators)).forEach(this.visitClassDecorator.bind(this)); super.visitClassDeclaration(declaration); @@ -115,22 +123,6 @@ export class NgWalker extends Lint.RuleWalker { } } - protected visitClassDecorator(decorator: ts.Decorator) { - let name = getDecoratorName(decorator); - - if (name === 'Injectable') { - this.visitNgInjectable(decorator.parent as ts.ClassDeclaration, decorator); - } - - if (name === 'Pipe') { - this.visitNgPipe(decorator.parent as ts.ClassDeclaration, decorator); - } - - if (name === 'NgModule') { - this.visitNgModule(decorator); - } - } - protected visitNgComponent(metadata: ComponentMetadata) { const { styles = [] } = metadata; @@ -165,13 +157,15 @@ export class NgWalker extends Lint.RuleWalker { } } - protected visitNgModule(decorator: ts.Decorator) {} + protected visitClassDecorator(decorator: ts.Decorator) {} + + protected visitNgModule(metadata: ModuleMetadata) {} protected visitNgDirective(metadata: DirectiveMetadata) {} - protected visitNgPipe(controller: ts.ClassDeclaration, decorator: ts.Decorator) {} + protected visitNgPipe(metadata: DirectiveMetadata) {} - protected visitNgInjectable(classDeclaration: ts.ClassDeclaration, decorator: ts.Decorator) {} + protected visitNgInjectable(metadata: InjectableMetadata) {} protected visitNgInput(property: ts.PropertyDeclaration, input: ts.Decorator, args: string[]) {} @@ -237,4 +231,13 @@ export class NgWalker extends Lint.RuleWalker { return sf; } + + private hasClassName(node: ts.Decorator | ts.ClassDeclaration) { + if (ts.isDecorator(node) && !getClassName(node.parent as ts.ClassDeclaration)) { + return false; + } else if (ts.isClassDeclaration(node) && !getClassName(node)) { + return false; + } + return true; + } } diff --git a/src/contextualLifecycleRule.ts b/src/contextualLifecycleRule.ts index 61c7313e1..e167b61de 100644 --- a/src/contextualLifecycleRule.ts +++ b/src/contextualLifecycleRule.ts @@ -14,6 +14,7 @@ import { MetadataTypeKeys, MetadataTypes } from './util/utils'; +import { InjectableMetadata, PipeMetadata } from './angular'; interface FailureParameters { readonly className: string; @@ -45,20 +46,18 @@ export class Rule extends AbstractRule { } class ContextualLifecycleWalker extends NgWalker { - protected visitNgInjectable(controller: ClassDeclaration, decorator: Decorator): void { - this.validateDecorator(controller, decorator, METADATA_TYPE_LIFECYCLE_MAPPER.Injectable); - super.visitNgInjectable(controller, decorator); + visitNgInjectable(metadata: InjectableMetadata): void { + this.validateDecorator(metadata.controller, metadata.decorator, METADATA_TYPE_LIFECYCLE_MAPPER.Injectable); + super.visitNgInjectable(metadata); } - protected visitNgPipe(controller: ClassDeclaration, decorator: Decorator): void { - this.validateDecorator(controller, decorator, METADATA_TYPE_LIFECYCLE_MAPPER.Pipe); - super.visitNgPipe(controller, decorator); + visitNgPipe(metadata: PipeMetadata): void { + this.validateDecorator(metadata.controller, metadata.decorator, METADATA_TYPE_LIFECYCLE_MAPPER.Pipe); + super.visitNgPipe(metadata); } private validateDecorator(controller: ClassDeclaration, decorator: Decorator, allowedMethods: ReadonlySet): void { - const className = getClassName(controller); - - if (!className) return; + const className = getClassName(controller)!; const metadataType = getDecoratorName(decorator); diff --git a/src/noPipeImpureRule.ts b/src/noPipeImpureRule.ts index 47b3419eb..35327b81b 100644 --- a/src/noPipeImpureRule.ts +++ b/src/noPipeImpureRule.ts @@ -4,6 +4,7 @@ import { AbstractRule } from 'tslint/lib/rules'; import { ClassDeclaration, Decorator, SourceFile, SyntaxKind } from 'typescript'; import { NgWalker } from './angular/ngWalker'; import { getClassName, getDecoratorPropertyInitializer } from './util/utils'; +import { PipeMetadata } from './angular'; interface FailureParameters { readonly className: string; @@ -31,13 +32,13 @@ export class Rule extends AbstractRule { } export class ClassMetadataWalker extends NgWalker { - protected visitNgPipe(controller: ClassDeclaration, decorator: Decorator): void { - this.validatePipe(controller, decorator); - super.visitNgPipe(controller, decorator); + protected visitNgPipe(metadata: PipeMetadata): void { + this.validatePipe(metadata); + super.visitNgPipe(metadata); } - private validatePipe(controller: ClassDeclaration, decorator: Decorator): void { - const pureExpression = getDecoratorPropertyInitializer(decorator, 'pure'); + private validatePipe(metadata: PipeMetadata): void { + const pureExpression = getDecoratorPropertyInitializer(metadata.decorator, 'pure'); if (!pureExpression) return; @@ -46,9 +47,7 @@ export class ClassMetadataWalker extends NgWalker { if (!parentExpression || isNotFalseLiteral) return; - const className = getClassName(controller); - - if (!className) return; + const className = getClassName(metadata.controller)!; const failure = getFailureMessage({ className }); diff --git a/src/pipePrefixRule.ts b/src/pipePrefixRule.ts index e175da409..a309523c0 100644 --- a/src/pipePrefixRule.ts +++ b/src/pipePrefixRule.ts @@ -4,6 +4,7 @@ import * as ts from 'typescript'; import { NgWalker } from './angular/ngWalker'; import { SelectorValidator } from './util/selectorValidator'; import { getDecoratorArgument } from './util/utils'; +import { PipeMetadata } from './angular'; export class Rule extends Lint.Rules.AbstractRule { static readonly metadata: Lint.IRuleMetadata = { @@ -69,10 +70,10 @@ export class ClassMetadataWalker extends NgWalker { super(sourceFile, rule.getOptions()); } - protected visitNgPipe(controller: ts.ClassDeclaration, decorator: ts.Decorator) { - let className = controller.name!.text; - this.validateProperties(className, decorator); - super.visitNgPipe(controller, decorator); + protected visitNgPipe(metadata: PipeMetadata) { + let className = metadata.controller.name!.text; + this.validateProperties(className, metadata.decorator); + super.visitNgPipe(metadata); } private validateProperties(className: string, pipe: ts.Decorator) { diff --git a/test/angular/metadataReader.spec.ts b/test/angular/metadataReader.spec.ts index bd54abef9..41a42a5ec 100644 --- a/test/angular/metadataReader.spec.ts +++ b/test/angular/metadataReader.spec.ts @@ -4,7 +4,7 @@ import * as ts from 'typescript'; import { Config } from '../../src/angular/config'; import { DummyFileResolver } from '../../src/angular/fileResolver/dummyFileResolver'; import { FsFileResolver } from '../../src/angular/fileResolver/fsFileResolver'; -import { ComponentMetadata } from '../../src/angular/metadata'; +import { ComponentMetadata, DirectiveMetadata, PipeMetadata } from '../../src/angular/metadata'; import { MetadataReader } from '../../src/angular/metadataReader'; import { join, normalize } from 'path'; @@ -25,7 +25,22 @@ describe('metadataReader', () => { const reader = new MetadataReader(new DummyFileResolver()); const ast = getAst(code); const metadata = reader.read(last(ast.statements) as ts.ClassDeclaration)!; - expect(metadata.selector).eq('foo'); + expect(metadata).instanceof(DirectiveMetadata); + expect((metadata as DirectiveMetadata).selector).eq('foo'); + }); + + it('should read name', () => { + const code = ` + @Pipe({ + name: 'foo' + }) + class Bar {} + `; + const reader = new MetadataReader(new DummyFileResolver()); + const ast = getAst(code); + const metadata = reader.read(last(ast.statements) as ts.ClassDeclaration)!; + expect(metadata).instanceof(PipeMetadata); + expect((metadata as PipeMetadata).name).eq('foo'); }); it('should not fail with empty decorator', () => { @@ -36,7 +51,8 @@ describe('metadataReader', () => { const reader = new MetadataReader(new DummyFileResolver()); const ast = getAst(code); const metadata = reader.read(last(ast.statements) as ts.ClassDeclaration)!; - expect(metadata.selector).eq(undefined); + expect(metadata).instanceof(DirectiveMetadata); + expect((metadata as DirectiveMetadata).selector).eq(undefined); }); it('should provide class declaration', () => { @@ -48,6 +64,7 @@ describe('metadataReader', () => { const ast = getAst(code); const classDeclaration = last(ast.statements); const metadata = reader.read(classDeclaration)!; + expect(metadata).instanceof(DirectiveMetadata); expect(metadata.controller).eq(classDeclaration); }); }); @@ -66,8 +83,8 @@ describe('metadataReader', () => { const ast = getAst(code); const classDeclaration = last(ast.statements); const metadata = reader.read(classDeclaration)!; - expect(metadata instanceof ComponentMetadata).eq(true); - expect(metadata.selector).eq('foo'); + expect(metadata).instanceof(ComponentMetadata); + expect((metadata as ComponentMetadata).selector).eq('foo'); const m = metadata; expect(m.template!.template.code).eq('bar'); expect(m.template!.url).eq(undefined); @@ -88,8 +105,8 @@ describe('metadataReader', () => { const ast = getAst(code); const classDeclaration = last(ast.statements); const metadata = reader.read(classDeclaration)!; - expect(metadata instanceof ComponentMetadata).eq(true); - expect(metadata.selector).eq('foo'); + expect(metadata).instanceof(ComponentMetadata); + expect((metadata as ComponentMetadata).selector).eq('foo'); const m = metadata; expect(m.template!.template.code).eq(''); expect(m.template!.url).eq('bar'); @@ -111,8 +128,8 @@ describe('metadataReader', () => { const ast = getAst(code); const classDeclaration = last(ast.statements); const metadata = reader.read(classDeclaration)!; - expect(metadata instanceof ComponentMetadata).eq(true); - expect(metadata.selector).eq('foo'); + expect(metadata).instanceof(ComponentMetadata); + expect((metadata as ComponentMetadata).selector).eq('foo'); const m = metadata; expect(m.template!.template.code).eq('qux'); expect(m.template!.url).eq(undefined); @@ -133,8 +150,8 @@ describe('metadataReader', () => { const ast = getAst(code, __dirname + '/../../test/fixtures/metadataReader/moduleid/foo.ts'); const classDeclaration = last(ast.statements); const metadata = reader.read(classDeclaration)!; - expect(metadata instanceof ComponentMetadata).eq(true); - expect(metadata.selector).eq('foo'); + expect(metadata).instanceof(ComponentMetadata); + expect((metadata as ComponentMetadata).selector).eq('foo'); const m = metadata; expect(m.template!.template.code.trim()).eq('
'); expect(m.template!.url!.endsWith('foo.html')).eq(true); @@ -155,8 +172,8 @@ describe('metadataReader', () => { const ast = getAst(code, __dirname + '/../../test/fixtures/metadataReader/moduleid/foo.ts'); const classDeclaration = last(ast.statements); const metadata = reader.read(classDeclaration)!; - expect(metadata instanceof ComponentMetadata).eq(true); - expect(metadata.selector).eq('foo'); + expect(metadata).instanceof(ComponentMetadata); + expect((metadata as ComponentMetadata).selector).eq('foo'); const m = metadata; expect(m.template!.template.code.trim()).eq('
'); expect(m.template!.url!.endsWith('foo.html')).eq(true); @@ -187,8 +204,8 @@ describe('metadataReader', () => { const classDeclaration = last(ast.statements); expect(invoked).eq(false); const metadata = reader.read(classDeclaration)!; - expect(metadata instanceof ComponentMetadata).eq(true); - expect(metadata.selector).eq('foo'); + expect(metadata).instanceof(ComponentMetadata); + expect((metadata as ComponentMetadata).selector).eq('foo'); const m = metadata; expect(m.template!.template.code.trim()).eq('
'); expect(m.template!.url!.endsWith('foo.html')).eq(true); @@ -223,8 +240,8 @@ describe('metadataReader', () => { const classDeclaration = last(ast.statements); expect(invoked).eq(false); const metadata = reader.read(classDeclaration)!; - expect(metadata instanceof ComponentMetadata).eq(true); - expect(metadata.selector).eq('foo'); + expect(metadata).instanceof(ComponentMetadata); + expect((metadata as ComponentMetadata).selector).eq('foo'); const m = metadata; expect(m.template!.template.code.trim()).eq('
'); expect(m.template!.url!.endsWith('foo.html')).eq(true); @@ -259,8 +276,8 @@ describe('metadataReader', () => { const classDeclaration = last(ast.statements); expect(invoked).eq(false); const metadata = reader.read(classDeclaration)!; - expect(metadata instanceof ComponentMetadata).eq(true); - expect(metadata.selector).eq('foo'); + expect(metadata).instanceof(ComponentMetadata); + expect((metadata as ComponentMetadata).selector).eq('foo'); const m = metadata; expect(m.template!.template.code.trim()).eq('
'); expect(m.template!.url!.endsWith('foo.html')).eq(true); @@ -300,8 +317,8 @@ describe('metadataReader', () => { const classDeclaration = last(ast.statements); expect(invoked).eq(false); const metadata = reader.read(classDeclaration)!; - expect(metadata instanceof ComponentMetadata).eq(true); - expect(metadata.selector).eq('foo'); + expect(metadata).instanceof(ComponentMetadata); + expect((metadata as ComponentMetadata).selector).eq('foo'); const m = metadata; expect(m.template!.template.code.trim()).eq('
'); expect(m.template!.url!.endsWith('foo.html')).eq(true); @@ -327,8 +344,8 @@ describe('metadataReader', () => { const ast = getAst(code, __dirname + '/../../test/fixtures/metadataReader/specialsymbols/foo.ts'); const classDeclaration = last(ast.statements); const metadata = reader.read(classDeclaration)!; - expect(metadata instanceof ComponentMetadata).eq(true); - expect(metadata.selector).eq('foo'); + expect(metadata).instanceof(ComponentMetadata); + expect((metadata as ComponentMetadata).selector).eq('foo'); const m = metadata; expect(m.template!.template.code.trim()).eq('
`
'); expect(m.template!.url!.endsWith('foo.html')).eq(true); @@ -350,8 +367,8 @@ describe('metadataReader', () => { const ast = getAst(code, __dirname + '/../../test/fixtures/metadataReader/notsupported/foo.ts'); const classDeclaration = last(ast.statements); const metadata = reader.read(classDeclaration)!; - expect(metadata instanceof ComponentMetadata).eq(true); - expect(metadata.selector).eq('foo'); + expect(metadata).instanceof(ComponentMetadata); + expect((metadata as ComponentMetadata).selector).eq('foo'); const m = metadata; expect(m.template!.template.code.trim()).eq(''); expect(m.template!.url!.endsWith('foo.dust')).eq(true); diff --git a/test/angular/ngWalker.spec.ts b/test/angular/ngWalker.spec.ts index 0f45b88f6..b7cf3449f 100644 --- a/test/angular/ngWalker.spec.ts +++ b/test/angular/ngWalker.spec.ts @@ -54,6 +54,47 @@ describe('ngWalker', () => { (chai.expect(injSpy).to.have.been as any).called(); }); + it('should not visit components, directives, pipes, injectables, and modules', () => { + let source = ` + @Component({ + selector: 'foo', + template: 'bar' + }) + export default class {} + @Directive({ + selector: '[baz]' + }) + export default class {} + @Pipe({ + name: 'foo' + }) + export default class {} + @NgModule({}) + export default class {} + @Injectable() + export default class {} + `; + let ruleArgs: Lint.IOptions = { + ruleName: 'foo', + ruleArguments: ['foo'], + disabledIntervals: [], + ruleSeverity: 'warning' + }; + let sf = ts.createSourceFile('foo', source, ts.ScriptTarget.ES5); + let walker = new NgWalker(sf, ruleArgs); + let cmpSpy = chaiSpy.on(walker, 'visitNgComponent'); + let dirSpy = chaiSpy.on(walker, 'visitNgDirective'); + let pipeSpy = chaiSpy.on(walker, 'visitNgPipe'); + let modSpy = chaiSpy.on(walker, 'visitNgModule'); + let injSpy = chaiSpy.on(walker, 'visitNgInjectable'); + walker.walk(sf); + (chai.expect(cmpSpy).to.not.have.been as any).called(); + (chai.expect(dirSpy).to.not.have.been as any).called(); + (chai.expect(pipeSpy).to.not.have.been as any).called(); + (chai.expect(modSpy).to.not.have.been as any).called(); + (chai.expect(injSpy).to.not.have.been as any).called(); + }); + it('should visit inputs and outputs with args', () => { let source = ` @Component({ From c9786b27711dd799be412ee427fcb6e90194e468 Mon Sep 17 00:00:00 2001 From: WilliamKoza Date: Sun, 10 Mar 2019 10:47:59 +0100 Subject: [PATCH 2/3] fixup! improve ngWalker by preventing an error when a class has no name. --- src/angular/metadata.ts | 30 +++++++++++++----------------- src/angular/metadataReader.ts | 28 +++++++++++++--------------- src/angular/ngWalker.ts | 9 ++------- src/contextualLifecycleRule.ts | 16 ++++++++-------- src/noOutputRenameRule.ts | 2 +- src/noPipeImpureRule.ts | 12 +++--------- src/util/utils.ts | 4 ++++ test/noPipeImpureRule.spec.ts | 2 +- 8 files changed, 45 insertions(+), 58 deletions(-) diff --git a/src/angular/metadata.ts b/src/angular/metadata.ts index 4b0a86f5d..367b06516 100644 --- a/src/angular/metadata.ts +++ b/src/angular/metadata.ts @@ -25,21 +25,17 @@ export interface TemplateMetadata extends PropertyMetadata { } export class DirectiveMetadata { - constructor( - public readonly controller: ts.ClassDeclaration, - public readonly decorator: ts.Decorator, - public readonly selector?: string - ) {} + constructor(readonly controller: ts.ClassDeclaration, readonly decorator: ts.Decorator, readonly selector?: string) {} } export class ComponentMetadata extends DirectiveMetadata { constructor( - public readonly controller: ts.ClassDeclaration, - public readonly decorator: ts.Decorator, - public readonly selector?: string, - public readonly animations?: (AnimationMetadata | undefined)[], - public readonly styles?: (StyleMetadata | undefined)[], - public readonly template?: TemplateMetadata + readonly controller: ts.ClassDeclaration, + readonly decorator: ts.Decorator, + readonly selector?: string, + readonly animations?: (AnimationMetadata | undefined)[], + readonly styles?: (StyleMetadata | undefined)[], + readonly template?: TemplateMetadata ) { super(controller, decorator, selector); } @@ -47,17 +43,17 @@ export class ComponentMetadata extends DirectiveMetadata { export class PipeMetadata { constructor( - public readonly controller: ts.ClassDeclaration, - public readonly decorator: ts.Decorator, - public readonly name?: string, - public readonly pure?: string + readonly controller: ts.ClassDeclaration, + readonly decorator: ts.Decorator, + readonly name?: string, + readonly pure?: ts.BooleanLiteral ) {} } export class ModuleMetadata { - constructor(public readonly controller: ts.ClassDeclaration, public readonly decorator: ts.Decorator) {} + constructor(readonly controller: ts.ClassDeclaration, readonly decorator: ts.Decorator) {} } export class InjectableMetadata { - constructor(public readonly controller: ts.ClassDeclaration, public readonly decorator: ts.Decorator) {} + constructor(readonly controller: ts.ClassDeclaration, readonly decorator: ts.Decorator) {} } diff --git a/src/angular/metadataReader.ts b/src/angular/metadataReader.ts index 1555beebd..9553df141 100644 --- a/src/angular/metadataReader.ts +++ b/src/angular/metadataReader.ts @@ -1,9 +1,9 @@ import * as ts from 'typescript'; -import { callExpression, decoratorArgument, getStringInitializerFromProperty, hasProperties, withIdentifier } from '../util/astQuery'; +import { callExpression, decoratorArgument, hasProperties, withIdentifier } from '../util/astQuery'; import { ifTrue, listToMaybe, Maybe, unwrapFirst } from '../util/function'; import { logger } from '../util/logger'; import { getAnimations, getInlineStyle, getTemplate } from '../util/ngQuery'; -import { getDecoratorPropertyInitializer, isStringLiteralLike, maybeNodeArray } from '../util/utils'; +import { getDecoratorPropertyInitializer, isBooleanLiteralLike, isStringLiteralLike, maybeNodeArray } from '../util/utils'; import { Config } from './config'; import { FileResolver } from './fileResolver/fileResolver'; import { @@ -38,7 +38,7 @@ export class MetadataReader { read(d: ts.ClassDeclaration): DirectiveMetadata | ComponentMetadata | PipeMetadata | ModuleMetadata | InjectableMetadata | undefined { const componentMetadata = unwrapFirst( - maybeNodeArray(d.decorators!).map(dec => { + maybeNodeArray(ts.createNodeArray(d.decorators)).map(dec => { return Maybe.lift(dec) .bind(callExpression) .bind(withIdentifier('Component') as any) @@ -47,7 +47,7 @@ export class MetadataReader { ); const directiveMetadata = unwrapFirst( - maybeNodeArray(d.decorators!).map(dec => + maybeNodeArray(ts.createNodeArray(d.decorators)).map(dec => Maybe.lift(dec) .bind(callExpression) .bind(withIdentifier('Directive') as any) @@ -56,7 +56,7 @@ export class MetadataReader { ); const pipeMetadata = unwrapFirst( - maybeNodeArray(d.decorators!).map(dec => + maybeNodeArray(ts.createNodeArray(d.decorators)).map(dec => Maybe.lift(dec) .bind(callExpression) .bind(withIdentifier('Pipe') as any) @@ -65,7 +65,7 @@ export class MetadataReader { ); const moduleMetadata = unwrapFirst( - maybeNodeArray(d.decorators!).map(dec => + maybeNodeArray(ts.createNodeArray(d.decorators)).map(dec => Maybe.lift(dec) .bind(callExpression) .bind(withIdentifier('NgModule') as any) @@ -74,7 +74,7 @@ export class MetadataReader { ); const injectableMetadata = unwrapFirst( - maybeNodeArray(d.decorators!).map(dec => + maybeNodeArray(ts.createNodeArray(d.decorators)).map(dec => Maybe.lift(dec) .bind(callExpression) .bind(withIdentifier('Injectable') as any) @@ -82,7 +82,7 @@ export class MetadataReader { ) ); - return directiveMetadata || componentMetadata || pipeMetadata || moduleMetadata || injectableMetadata || undefined; + return directiveMetadata || componentMetadata || pipeMetadata || moduleMetadata || injectableMetadata; } protected readDirectiveMetadata(d: ts.ClassDeclaration, dec: ts.Decorator): DirectiveMetadata { @@ -93,15 +93,13 @@ export class MetadataReader { } protected readPipeMetadata(d: ts.ClassDeclaration, dec: ts.Decorator): DirectiveMetadata { - const name = this.getDecoratorArgument(dec) - .bind(expr => getStringInitializerFromProperty('name', expr!.properties)) - .fmap(initializer => initializer!.text); + const nameExpression = getDecoratorPropertyInitializer(dec, 'name'); + const name = nameExpression && isStringLiteralLike(nameExpression) ? nameExpression.text : undefined; - const pure = this.getDecoratorArgument(dec) - .bind(expr => getStringInitializerFromProperty('pure', expr!.properties)) - .fmap(initializer => initializer!.text); + const pureExpression = getDecoratorPropertyInitializer(dec, 'pure'); + const isBoolean = pureExpression && isBooleanLiteralLike(pureExpression); - return new PipeMetadata(d, dec, name.unwrap(), pure ? pure.unwrap() : undefined); + return new PipeMetadata(d, dec, name, isBoolean ? (pureExpression as ts.BooleanLiteral) : undefined); } protected readModuleMetadata(d: ts.ClassDeclaration, dec: ts.Decorator): DirectiveMetadata { diff --git a/src/angular/ngWalker.ts b/src/angular/ngWalker.ts index 114dafcfe..1f495ba48 100644 --- a/src/angular/ngWalker.ts +++ b/src/angular/ngWalker.ts @@ -163,7 +163,7 @@ export class NgWalker extends Lint.RuleWalker { protected visitNgDirective(metadata: DirectiveMetadata) {} - protected visitNgPipe(metadata: DirectiveMetadata) {} + protected visitNgPipe(metadata: PipeMetadata) {} protected visitNgInjectable(metadata: InjectableMetadata) {} @@ -233,11 +233,6 @@ export class NgWalker extends Lint.RuleWalker { } private hasClassName(node: ts.Decorator | ts.ClassDeclaration) { - if (ts.isDecorator(node) && !getClassName(node.parent as ts.ClassDeclaration)) { - return false; - } else if (ts.isClassDeclaration(node) && !getClassName(node)) { - return false; - } - return true; + return (ts.isDecorator(node) && getClassName(node.parent)) || (ts.isClassDeclaration(node) && getClassName(node)); } } diff --git a/src/contextualLifecycleRule.ts b/src/contextualLifecycleRule.ts index e167b61de..15bb4e55b 100644 --- a/src/contextualLifecycleRule.ts +++ b/src/contextualLifecycleRule.ts @@ -46,24 +46,24 @@ export class Rule extends AbstractRule { } class ContextualLifecycleWalker extends NgWalker { - visitNgInjectable(metadata: InjectableMetadata): void { - this.validateDecorator(metadata.controller, metadata.decorator, METADATA_TYPE_LIFECYCLE_MAPPER.Injectable); + protected visitNgInjectable(metadata: InjectableMetadata): void { + this.validateDecorator(metadata, METADATA_TYPE_LIFECYCLE_MAPPER.Injectable); super.visitNgInjectable(metadata); } - visitNgPipe(metadata: PipeMetadata): void { - this.validateDecorator(metadata.controller, metadata.decorator, METADATA_TYPE_LIFECYCLE_MAPPER.Pipe); + protected visitNgPipe(metadata: PipeMetadata): void { + this.validateDecorator(metadata, METADATA_TYPE_LIFECYCLE_MAPPER.Pipe); super.visitNgPipe(metadata); } - private validateDecorator(controller: ClassDeclaration, decorator: Decorator, allowedMethods: ReadonlySet): void { - const className = getClassName(controller)!; + private validateDecorator(metadata: PipeMetadata, allowedMethods: ReadonlySet): void { + const className = getClassName(metadata.controller)!; - const metadataType = getDecoratorName(decorator); + const metadataType = getDecoratorName(metadata.decorator); if (!metadataType || !isMetadataType(metadataType)) return; - for (const member of controller.members) { + for (const member of metadata.controller.members) { const { name: memberName } = member; if (!memberName) continue; diff --git a/src/noOutputRenameRule.ts b/src/noOutputRenameRule.ts index e842bf020..084998dea 100644 --- a/src/noOutputRenameRule.ts +++ b/src/noOutputRenameRule.ts @@ -29,7 +29,7 @@ export const getFailureMessage = (): string => { export class OutputMetadataWalker extends NgWalker { private directiveSelectors!: ReadonlySet; - visitNgDirective(metadata: DirectiveMetadata): void { + protected visitNgDirective(metadata: DirectiveMetadata): void { this.directiveSelectors = new Set((metadata.selector || '').replace(/[\[\]\s]/g, '').split(',')); super.visitNgDirective(metadata); } diff --git a/src/noPipeImpureRule.ts b/src/noPipeImpureRule.ts index 35327b81b..428ca7a48 100644 --- a/src/noPipeImpureRule.ts +++ b/src/noPipeImpureRule.ts @@ -38,19 +38,13 @@ export class ClassMetadataWalker extends NgWalker { } private validatePipe(metadata: PipeMetadata): void { - const pureExpression = getDecoratorPropertyInitializer(metadata.decorator, 'pure'); - - if (!pureExpression) return; - - const { parent: parentExpression } = pureExpression; - const isNotFalseLiteral = pureExpression.kind !== SyntaxKind.FalseKeyword; - - if (!parentExpression || isNotFalseLiteral) return; + if (!metadata.pure) return; + if (metadata.pure!.kind !== SyntaxKind.FalseKeyword) return; const className = getClassName(metadata.controller)!; const failure = getFailureMessage({ className }); - this.addFailureAtNode(parentExpression, failure); + this.addFailureAtNode(metadata.pure, failure); } } diff --git a/src/util/utils.ts b/src/util/utils.ts index 2298e7ab0..655001783 100644 --- a/src/util/utils.ts +++ b/src/util/utils.ts @@ -19,6 +19,7 @@ import { ObjectLiteralExpression, SourceFile, StringLiteral, + BooleanLiteral, SyntaxKind } from 'typescript'; import { getDeclaredMethods } from './classDeclarationUtils'; @@ -202,6 +203,9 @@ export const isSameLine = (sourceFile: SourceFile, pos1: number, pos2: number): export const isStringLiteralLike = (node: Node): node is StringLiteral | NoSubstitutionTemplateLiteral => isStringLiteral(node) || isNoSubstitutionTemplateLiteral(node); +export const isBooleanLiteralLike = (node: Node): node is BooleanLiteral => + node.kind === SyntaxKind.FalseKeyword || node.kind === SyntaxKind.TrueKeyword; + export const maybeNodeArray = (nodes: NodeArray): ReadonlyArray => nodes || []; // Regex below matches any Unicode word and compatible with ES5. In ES2018 the same result diff --git a/test/noPipeImpureRule.spec.ts b/test/noPipeImpureRule.spec.ts index 6c11bb6bd..5feb0f609 100644 --- a/test/noPipeImpureRule.spec.ts +++ b/test/noPipeImpureRule.spec.ts @@ -12,7 +12,7 @@ describe(ruleName, () => { @Pipe({ name: 'test', pure: false - ~~~~~~~~~~~ + ~~~~~ }) class Test {} `; From a9aeed5051144f945d541f9a45edafab8ac970e1 Mon Sep 17 00:00:00 2001 From: WilliamKoza Date: Sun, 10 Mar 2019 18:47:12 +0100 Subject: [PATCH 3/3] fixup! improve ngWalker by preventing an error when a class has no name. --- src/angular/metadataReader.ts | 4 ++-- src/noPipeImpureRule.ts | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/angular/metadataReader.ts b/src/angular/metadataReader.ts index 9553df141..06d659e64 100644 --- a/src/angular/metadataReader.ts +++ b/src/angular/metadataReader.ts @@ -97,9 +97,9 @@ export class MetadataReader { const name = nameExpression && isStringLiteralLike(nameExpression) ? nameExpression.text : undefined; const pureExpression = getDecoratorPropertyInitializer(dec, 'pure'); - const isBoolean = pureExpression && isBooleanLiteralLike(pureExpression); + const pure = pureExpression && isBooleanLiteralLike(pureExpression) ? pureExpression : undefined; - return new PipeMetadata(d, dec, name, isBoolean ? (pureExpression as ts.BooleanLiteral) : undefined); + return new PipeMetadata(d, dec, name, pure); } protected readModuleMetadata(d: ts.ClassDeclaration, dec: ts.Decorator): DirectiveMetadata { diff --git a/src/noPipeImpureRule.ts b/src/noPipeImpureRule.ts index 428ca7a48..0d0fca0a1 100644 --- a/src/noPipeImpureRule.ts +++ b/src/noPipeImpureRule.ts @@ -38,8 +38,7 @@ export class ClassMetadataWalker extends NgWalker { } private validatePipe(metadata: PipeMetadata): void { - if (!metadata.pure) return; - if (metadata.pure!.kind !== SyntaxKind.FalseKeyword) return; + if (!metadata.pure || metadata.pure.kind !== SyntaxKind.FalseKeyword) return; const className = getClassName(metadata.controller)!;