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

Jump to bottom

refactor: use type guard instead of unnecessary or incorrect type assertions #659

Merged
merged 1 commit into from
Jun 12, 2018
Merged

refactor: use type guard instead of unnecessary or incorrect type assertions #659

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 7 additions & 15 deletions src/angular/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ export interface DirectiveDeclaration {

let BUILD_TYPE = '<%= BUILD_TYPE %>';

const transform = (code: string, extension: '.css' | '.html', url?: string): { code: string; url?: string } => {
return { code: !url || url.endsWith(extension) ? code : '', url };
};

export const Config: Config = {
interpolation: ['{{', '}}'],

Expand Down Expand Up @@ -71,25 +75,13 @@ export const Config: Config = {
return url;
},

transformStyle(code: string, url?: string) {
if (!url || url.endsWith('.css')) {
return { code, url };
}

return { code: '', url };
},

transformTemplate(code: string, url?: string) {
if (!url || url.endsWith('.html')) {
return { code, url };
}
transformStyle: (code: string, url?: string) => transform(code, '.css', url),

return { code: '', url };
}
transformTemplate: (code: string, url?: string) => transform(code, '.html', url)
};

try {
const root = require('app-root-path');
const newConfig = require(root.path + '/.codelyzer');
Object.assign(Config, newConfig);
} catch (e) {}
} catch {}
21 changes: 15 additions & 6 deletions src/angular/metadata.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,22 @@ export interface TemplateMetadata extends PropertyMetadata {
}

export class DirectiveMetadata {
controller!: ts.ClassDeclaration;
decorator!: ts.Decorator;
selector!: string;
constructor(
public readonly controller: ts.ClassDeclaration,
public readonly decorator: ts.Decorator,
public readonly selector?: string
) {}
}

export class ComponentMetadata extends DirectiveMetadata {
animations!: AnimationMetadata[];
styles!: StyleMetadata[];
template!: TemplateMetadata;
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
) {
super(controller, decorator, selector);
}
}
42 changes: 17 additions & 25 deletions src/angular/metadataReader.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,9 @@
import * as ts from 'typescript';
import {
callExpression,
decoratorArgument,
getStringInitializerFromProperty,
hasProperties,
isSimpleTemplateString,
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 { maybeNodeArray } from '../util/utils';
import { isSimpleTemplateString, maybeNodeArray } from '../util/utils';
import { Config } from './config';
import { FileResolver } from './fileResolver/fileResolver';
import { AnimationMetadata, CodeWithSourceMap, ComponentMetadata, DirectiveMetadata, StyleMetadata, TemplateMetadata } from './metadata';
Expand Down Expand Up @@ -60,38 +53,37 @@ export class MetadataReader {
.bind(expr => getStringInitializerFromProperty('selector', expr!.properties))
.fmap(initializer => initializer!.text);

return Object.assign(new DirectiveMetadata(), {
controller: d,
decorator: dec,
selector: selector.unwrap()
});
return new DirectiveMetadata(d, dec, selector.unwrap());
}

protected readComponentMetadata(d: ts.ClassDeclaration, dec: ts.Decorator): ComponentMetadata {
const expr = this.getDecoratorArgument(dec);
const directiveMetadata = this.readDirectiveMetadata(d, dec);
const external_M = expr.fmap(() => this._urlResolver!.resolve(dec));
const animations_M = external_M.bind(external => this.readComponentAnimationsMetadata(dec, external!));
const animations_M = external_M.bind(() => this.readComponentAnimationsMetadata(dec));
const style_M = external_M.bind(external => this.readComponentStylesMetadata(dec, external!));
const template_M = external_M.bind(external => this.readComponentTemplateMetadata(dec, external!));

return Object.assign(new ComponentMetadata(), directiveMetadata, {
animations: animations_M.unwrap(),
styles: style_M.unwrap(),
template: template_M.unwrap()
});
return new ComponentMetadata(
directiveMetadata.controller,
directiveMetadata.decorator,
directiveMetadata.selector,
animations_M.unwrap(),
style_M.unwrap(),
template_M.unwrap()
);
}

protected getDecoratorArgument(decorator: ts.Decorator): Maybe<ts.ObjectLiteralExpression | undefined> {
return decoratorArgument(decorator).bind(ifTrue(hasProperties));
}

protected readComponentAnimationsMetadata(dec: ts.Decorator, external: MetadataUrls): Maybe<AnimationMetadata[] | undefined> {
protected readComponentAnimationsMetadata(dec: ts.Decorator): Maybe<(AnimationMetadata | undefined)[] | undefined> {
return getAnimations(dec).fmap(inlineAnimations =>
inlineAnimations!.elements
.filter(inlineAnimation => isSimpleTemplateString(inlineAnimation))
.map<AnimationMetadata>(inlineAnimation => ({
animation: normalizeTransformed({ code: (inlineAnimation as ts.StringLiteral | ts.NoSubstitutionTemplateLiteral).text }),
animation: normalizeTransformed({ code: (inlineAnimation as ts.StringLiteralLike).text }),
node: inlineAnimation as ts.Node
}))
);
Expand Down Expand Up @@ -122,8 +114,8 @@ export class MetadataReader {
.fmap(inlineStyles =>
// Resolve Inline styles
inlineStyles!.elements.filter(inlineStyle => isSimpleTemplateString(inlineStyle)).map<StyleMetadata>(inlineStyle => ({
node: inlineStyle as ts.Node,
style: normalizeTransformed(Config.transformStyle((inlineStyle as ts.StringLiteral | ts.NoSubstitutionTemplateLiteral).text))
node: inlineStyle,
style: normalizeTransformed(Config.transformStyle((inlineStyle as ts.StringLiteralLike).text))
}))
)
.catch(() =>
Expand All @@ -148,7 +140,7 @@ export class MetadataReader {
private _resolve(url: string): Maybe<string | undefined> {
try {
return Maybe.lift(this._fileResolver.resolve(url));
} catch (e) {
} catch {
logger.info('Cannot read file' + url);
return Maybe.nothing;
}
Expand Down
99 changes: 51 additions & 48 deletions src/angular/ngWalker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,22 @@ import { RecursiveAngularExpressionVisitor } from './templates/recursiveAngularE
import { ReferenceCollectorVisitor } from './templates/referenceCollectorVisitor';
import { parseTemplate } from './templates/templateParser';

const getDecoratorStringArgs = (decorator: ts.Decorator): (string | null)[] => {
const getDecoratorStringArgs = (decorator: ts.Decorator): string[] => {
const { expression } = decorator;
const args = ts.isCallExpression(expression) ? expression.arguments : ts.createNodeArray();

return args.map(a => (ts.isStringLiteral(a) ? a.text : null));
return args.filter(ts.isStringLiteral).map(x => x.text);
};

const getPosition = (node: ts.Node) => {
let pos = 0;
if (node) {
pos = node.pos + 1;
try {
pos = node.getStart() + 1;
} catch {}
}
return pos;
};

export interface NgWalkerConfig {
Expand Down Expand Up @@ -55,50 +66,50 @@ export class NgWalker extends Lint.RuleWalker {
} else if (metadata instanceof DirectiveMetadata) {
this.visitNgDirective(metadata);
}
maybeNodeArray(<ts.NodeArray<ts.Decorator>>declaration.decorators).forEach(this.visitClassDecorator.bind(this));
maybeNodeArray(ts.createNodeArray(declaration.decorators)).forEach(this.visitClassDecorator.bind(this));
super.visitClassDeclaration(declaration);
}

visitMethodDeclaration(method: ts.MethodDeclaration) {
maybeNodeArray(<ts.NodeArray<ts.Decorator>>method.decorators).forEach(this.visitMethodDecorator.bind(this));
maybeNodeArray(ts.createNodeArray(method.decorators)).forEach(this.visitMethodDecorator.bind(this));
super.visitMethodDeclaration(method);
}

visitPropertyDeclaration(prop: ts.PropertyDeclaration) {
maybeNodeArray(<ts.NodeArray<ts.Decorator>>prop.decorators).forEach(this.visitPropertyDecorator.bind(this));
maybeNodeArray(ts.createNodeArray(prop.decorators)).forEach(this.visitPropertyDecorator.bind(this));
super.visitPropertyDeclaration(prop);
}

protected visitMethodDecorator(decorator: ts.Decorator) {
let name = getDecoratorName(decorator);
if (name === 'HostListener') {
this.visitNgHostListener(<ts.MethodDeclaration>decorator.parent, decorator, getDecoratorStringArgs(decorator));
this.visitNgHostListener(decorator.parent as ts.MethodDeclaration, decorator, getDecoratorStringArgs(decorator));
}
}

protected visitPropertyDecorator(decorator: ts.Decorator) {
let name = getDecoratorName(decorator);
switch (name) {
case 'Input':
this.visitNgInput(<ts.PropertyDeclaration>decorator.parent, decorator, getDecoratorStringArgs(decorator));
this.visitNgInput(decorator.parent as ts.PropertyDeclaration, decorator, getDecoratorStringArgs(decorator));
break;
case 'Output':
this.visitNgOutput(<ts.PropertyDeclaration>decorator.parent, decorator, getDecoratorStringArgs(decorator));
this.visitNgOutput(decorator.parent as ts.PropertyDeclaration, decorator, getDecoratorStringArgs(decorator));
break;
case 'HostBinding':
this.visitNgHostBinding(<ts.PropertyDeclaration>decorator.parent, decorator, getDecoratorStringArgs(decorator));
this.visitNgHostBinding(decorator.parent as ts.PropertyDeclaration, decorator, getDecoratorStringArgs(decorator));
break;
case 'ContentChild':
this.visitNgContentChild(<ts.PropertyDeclaration>decorator.parent, decorator, getDecoratorStringArgs(decorator));
this.visitNgContentChild(decorator.parent as ts.PropertyDeclaration, decorator, getDecoratorStringArgs(decorator));
break;
case 'ContentChildren':
this.visitNgContentChildren(<ts.PropertyDeclaration>decorator.parent, decorator, getDecoratorStringArgs(decorator));
this.visitNgContentChildren(decorator.parent as ts.PropertyDeclaration, decorator, getDecoratorStringArgs(decorator));
break;
case 'ViewChild':
this.visitNgViewChild(<ts.PropertyDeclaration>decorator.parent, decorator, getDecoratorStringArgs(decorator));
this.visitNgViewChild(decorator.parent as ts.PropertyDeclaration, decorator, getDecoratorStringArgs(decorator));
break;
case 'ViewChildren':
this.visitNgViewChildren(<ts.PropertyDeclaration>decorator.parent, decorator, getDecoratorStringArgs(decorator));
this.visitNgViewChildren(decorator.parent as ts.PropertyDeclaration, decorator, getDecoratorStringArgs(decorator));
break;
}
}
Expand All @@ -107,56 +118,48 @@ export class NgWalker extends Lint.RuleWalker {
let name = getDecoratorName(decorator);

if (name === 'Injectable') {
this.visitNgInjectable(<ts.ClassDeclaration>decorator.parent, decorator);
this.visitNgInjectable(decorator.parent as ts.ClassDeclaration, decorator);
}

if (name === 'Pipe') {
this.visitNgPipe(<ts.ClassDeclaration>decorator.parent, decorator);
this.visitNgPipe(decorator.parent as ts.ClassDeclaration, decorator);
}

if (name === 'NgModule') {
this.visitNgModule(decorator);
}

// Not invoked @Component or @Pipe, or @Directive
if (
!(<ts.CallExpression>decorator.expression).arguments ||
!(<ts.CallExpression>decorator.expression).arguments.length ||
!(<ts.ObjectLiteralExpression>(<ts.CallExpression>decorator.expression).arguments[0]).properties
) {
return;
}
}

protected visitNgComponent(metadata: ComponentMetadata) {
const template = metadata.template;
const getPosition = (node: ts.Node) => {
let pos = 0;
if (node) {
pos = node.pos + 1;
try {
pos = node.getStart() + 1;
} catch (e) {}
}
return pos;
};

const { styles = [] } = metadata;

for (const style of styles) {
try {
this.visitNgStyleHelper(parseCss(style.style.code), metadata, style, getPosition(style.node!));
const cssAst = parseCss(style!.style.code);
this.visitNgStyleHelper(cssAst, metadata, style!, getPosition(style!.node!));
} catch (e) {
logger.error('Cannot parse the styles of', ((<any>metadata.controller || {}).name || {}).text, e);
const {
controller: { name }
} = metadata;
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs defaults in order to guard the same way the original did. Not sure if intentional.

Copy link
Collaborator Author

@rafaelss95 rafaelss95 Jun 3, 2018 •
edited

Choose a reason for hiding this comment

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

The property controller always exists according to:

codelyzer/src/angular/metadata.ts

Lines 23 to 27 in da0f553

export class DirectiveMetadata {
controller!: ts.ClassDeclaration;
decorator!: ts.Decorator;
selector!: string;
}

... and it makes sense, because once we instantiate a DirectiveMetadata, we pass all properties:

codelyzer/src/angular/metadataReader.ts

Lines 63 to 67 in da0f553

return Object.assign(new DirectiveMetadata(), {
controller: d,
decorator: dec,
selector: selector.unwrap()
});

... now I'm curious on why we are using Object.assign instead of a trivial constructor. Thoughts?

Anyway, these "defaults" should have been changed in #631, but I really forgot about it. 😞

const text = name && ts.isIdentifier(name) ? name.text : '';

logger.error('Cannot parse the styles of', text, e);
}
}

const { template } = metadata;

if (template && template.template) {
try {
const templateAst = parseTemplate(template.template.code, Config.predefinedDirectives);
this.visitNgTemplateHelper(templateAst, metadata, getPosition(template.node!));
} catch (e) {
logger.error('Cannot parse the template of', ((<any>metadata.controller || {}).name || {}).text, e);
const {
controller: { name }
} = metadata;
const text = name && ts.isIdentifier(name) ? name.text : '';

logger.error('Cannot parse the template of', text, e);
}
}
}
Expand All @@ -169,28 +172,28 @@ export class NgWalker extends Lint.RuleWalker {

protected visitNgInjectable(classDeclaration: ts.ClassDeclaration, decorator: ts.Decorator) {}

protected visitNgInput(property: ts.PropertyDeclaration, input: ts.Decorator, args: (string | null)[]) {}
protected visitNgInput(property: ts.PropertyDeclaration, input: ts.Decorator, args: string[]) {}

protected visitNgOutput(property: ts.PropertyDeclaration, output: ts.Decorator, args: (string | null)[]) {}
protected visitNgOutput(property: ts.PropertyDeclaration, output: ts.Decorator, args: string[]) {}

protected visitNgHostBinding(property: ts.PropertyDeclaration, decorator: ts.Decorator, args: (string | null)[]) {}
protected visitNgHostBinding(property: ts.PropertyDeclaration, decorator: ts.Decorator, args: string[]) {}

protected visitNgHostListener(method: ts.MethodDeclaration, decorator: ts.Decorator, args: (string | null)[]) {}
protected visitNgHostListener(method: ts.MethodDeclaration, decorator: ts.Decorator, args: string[]) {}

protected visitNgContentChild(property: ts.PropertyDeclaration, input: ts.Decorator, args: (string | null)[]) {}
protected visitNgContentChild(property: ts.PropertyDeclaration, input: ts.Decorator, args: string[]) {}

protected visitNgContentChildren(property: ts.PropertyDeclaration, input: ts.Decorator, args: (string | null)[]) {}
protected visitNgContentChildren(property: ts.PropertyDeclaration, input: ts.Decorator, args: string[]) {}

protected visitNgViewChild(property: ts.PropertyDeclaration, input: ts.Decorator, args: (string | null)[]) {}
protected visitNgViewChild(property: ts.PropertyDeclaration, input: ts.Decorator, args: string[]) {}

protected visitNgViewChildren(property: ts.PropertyDeclaration, input: ts.Decorator, args: (string | null)[]) {}
protected visitNgViewChildren(property: ts.PropertyDeclaration, input: ts.Decorator, args: string[]) {}

protected visitNgTemplateHelper(roots: compiler.TemplateAst[], context: ComponentMetadata, baseStart: number) {
if (!roots || !roots.length) {
return;
}

const sourceFile = this.getContextSourceFile(context.template.url!, context.template.template.source!);
const sourceFile = this.getContextSourceFile(context.template!.url!, context.template!.template.source!);
const referenceVisitor = new ReferenceCollectorVisitor();
const visitor = new this._config!.templateVisitorCtrl!(
sourceFile,
Expand Down
4 changes: 2 additions & 2 deletions src/angular/sourceMappingVisitor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ function computeLineAndCharacterOfPosition(lineStarts: number[], position: numbe
}

function computeLineStarts(text: string): number[] {
const result: number[] = new Array();
const result: number[] = [];
let pos = 0;
let lineStart = 0;
while (pos < text.length) {
Expand Down Expand Up @@ -95,7 +95,7 @@ export class SourceMappingVisitor extends RuleWalker {
parentAST: any;
private consumer: SourceMapConsumer;

constructor(sourceFile: ts.SourceFile, options: IOptions, protected codeWithMap: CodeWithSourceMap, protected basePosition: number) {
constructor(sourceFile: ts.SourceFile, options: IOptions, public codeWithMap: CodeWithSourceMap, protected basePosition: number) {
Copy link
Owner

Choose a reason for hiding this comment

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

Why not protected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It was necessary because I've changed the type of context in trackByRule#35 from any to BasicTemplateAstVisitor (https://github.com/mgechev/codelyzer/pull/659/files#diff-26d9296d5e0a25d3fe6a9b174e67da35R35)... it gives an error with protected:

src/trackByFunctionRule.ts(44,30): error TS2446: Property 'codeWithMap' is protected and only accessible through an instance of class 'TrackByFunctionTemplateVisitor'.

super(sourceFile, options);
if (this.codeWithMap.map) {
this.consumer = new SourceMapConsumer(this.codeWithMap.map);
Expand Down
2 changes: 1 addition & 1 deletion src/angular/styles/cssAst.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ export class CssBlockDefinitionRuleAst extends CssBlockRuleAst {
block: CssBlockAst
) {
super(location, type, block);
const firstCssToken: CssToken = query.tokens[0];
const firstCssToken = query.tokens[0];
this.name = new CssToken(firstCssToken.index, firstCssToken.column, firstCssToken.line, CssTokenType.Identifier, this.strValue);
}
visit(visitor: CssAstVisitor, context?: any): any {
Expand Down