Skip to content

Commit

Permalink
feat(binding): basic custom element checking
Browse files Browse the repository at this point in the history
ref #67
  • Loading branch information
MeirionHughes committed Oct 8, 2016
1 parent f286fe3 commit 26316d4
Show file tree
Hide file tree
Showing 6 changed files with 121 additions and 12 deletions.
44 changes: 42 additions & 2 deletions source/ast.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { AccessMember, AccessScope, AccessKeyed, NameExpression, ValueConverter
import { Container } from 'aurelia-dependency-injection';
import { Rule, Parser, ParserState, Issue, IssueSeverity } from 'template-lint';
import ts = require('typescript');
import * as path from 'path';


import {
Expand All @@ -15,21 +16,28 @@ import {
from 'aurelia-templating';
import { ASTAttribute as P5ASTAttribute } from "parse5";

import { Reflection } from './reflection';
import { AureliaReflection } from './aurelia-reflection';

export class ASTBuilder extends Rule {
public root: ASTNode;
public resources: Array<{ name: string, type: ts.DeclarationStatement, kind: string }>;
public reportBindingSyntax = true;
private basePath: string;

constructor(protected auReflection?: AureliaReflection) {

constructor(protected reflection: Reflection, protected auReflection?: AureliaReflection) {
super();
this.auReflection = this.auReflection || new AureliaReflection();
this.resources = [];
}

init(parser: Parser) {
init(parser: Parser, filepath: string) {
var current = new ASTNode();
this.root = current;

this.basePath = filepath ? path.dirname(filepath) : "";

parser.on("startTag", (tag, attrs, selfClosing, loc) => {
let next = new ASTElementNode();
next.tag = tag;
Expand All @@ -49,6 +57,37 @@ export class ASTBuilder extends Rule {
current.children.push(next);
if (!parser.isVoid(tag))
current = next;

//triage #67
if (tag === "require") {
var from = attrs.find(x => x.name == "from");

if (from == null || from.value == null || from.value == "")
return;

This comment has been minimized.

Copy link
@atsu85

atsu85 Oct 8, 2016

Contributor

Shouldn't it report a problem instead of silently returning? Perhaps it is more of a responsibility of RequireRule to report this type of problem, but I understand it might be easier to handle it here.

This comment has been minimized.

Copy link
@MeirionHughes

MeirionHughes Oct 8, 2016

Author Contributor

yeah it probably makes sense to do it in one place.


var lookup = path.normalize(path.join(this.basePath, from.value));
var source = <ts.SourceFile>this.reflection.pathToSource[lookup];

if (source == null)
return;

This comment has been minimized.

Copy link
@atsu85

atsu85 Oct 8, 2016

Contributor

Shouldn't it report a problem instead of silently returning?

This comment has been minimized.

Copy link
@MeirionHughes

MeirionHughes Oct 8, 2016

Author Contributor

yup. :P


var customElement = <ts.ClassDeclaration>source.statements.find(
(x) => {
return x.kind == ts.SyntaxKind.ClassDeclaration &&
(<ts.ClassDeclaration>x).name.getText().endsWith("CustomElement");

This comment has been minimized.

Copy link
@atsu85

atsu85 Oct 8, 2016

Contributor

Actually class doesn't need to have CustomElement suffix, I've been using following convention for custom elements:

  1. html/TS file name: projectPrefix + "-" + customElementNameInKebabCase + "." + extension
  2. class name: toUpperCamelCase(projectPrefix + "-" + customElementNameInKebabCase)

Works like a charm :)

This comment has been minimized.

Copy link
@MeirionHughes

MeirionHughes Oct 8, 2016

Author Contributor

so if the file name is <blah>-custom-element, that works too?

This comment has been minimized.

Copy link
@atsu85

atsu85 Oct 8, 2016

Contributor

An example from my app: my-login.html, my-login.ts, export class MyLogin

This comment has been minimized.

Copy link
@atsu85

atsu85 Oct 8, 2016

Contributor

And i don't need to add decorator to tell the name of custom element

This comment has been minimized.

Copy link
@MeirionHughes

MeirionHughes Oct 8, 2016

Author Contributor

presumably it assumes first export is a custom element then. :/

This comment has been minimized.

Copy link
@atsu85

atsu85 Oct 8, 2016

Contributor

Only in case of routable components, but not for custom elements - they seem to be matched by tag or file name... remember this comment?

This comment has been minimized.

Copy link
@MeirionHughes

MeirionHughes Oct 8, 2016

Author Contributor

yeah; I think the only way to do it would be to reimplement the module resolution class, but with reflection (via ts) rather than actual module values. only way to get it robust enough I think.

});

if (!customElement)
return;

var res = {
kind: "custom-element",
name: this.auReflection.customElementToDash((<ts.ClassDeclaration>customElement).name.getText()),
type: customElement
};

this.resources.push(res);
};
});

parser.on("endTag", (tag, attrs, selfClosing, loc) => {
Expand Down Expand Up @@ -207,6 +246,7 @@ export class ASTAttribute {
export class ASTElementNode extends ASTNode {
public tag: string;
public attrs: ASTAttribute[];
public typeDecl: ts.DeclarationStatement;

constructor() {
super();
Expand Down
10 changes: 10 additions & 0 deletions source/aurelia-reflection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,14 @@ export class AureliaReflection {
exp = this.bindingLanguage.inspectTextContent(this.resources, text);
return exp;
}

toDashCase(value: string) {
return value.replace(/([a-z][A-Z])/g, function (g) { return g[0] + "-" + g[1].toLowerCase(); });
}

customElementToDash(value: string) {
if (value.endsWith("CustomElement"))
value = value.substring(0, value.length - "CustomElement".length);
return this.toDashCase(value.charAt(0).toLowerCase() + value.slice(1));
}
}
34 changes: 27 additions & 7 deletions source/rules/binding.ts
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export class BindingRule extends ASTBuilder {
public restrictedAccess = ["private", "protected"];

constructor(
private reflection: Reflection,
reflection: Reflection,
auReflection: AureliaReflection,
opt?: {
reportBindingSyntax?: boolean,
Expand All @@ -47,16 +47,15 @@ export class BindingRule extends ASTBuilder {
localProvidors?: string[],
restrictedAccess?: string[]
}) {

super(auReflection);
super(reflection, auReflection);

if (opt)
Object.assign(this, opt);
}

init(parser: Parser, path?: string) {
super.init(parser);
this.root.context = this.resolveViewModel(path);
init(parser: Parser, filepath?: string) {
super.init(parser, filepath);
this.root.context = this.resolveViewModel(filepath);
}

finalise(): Issue[] {
Expand Down Expand Up @@ -94,6 +93,13 @@ export class BindingRule extends ASTBuilder {
}

private examineElementNode(node: ASTElementNode) {

var customResource = this.resources.find(x => x.name == node.tag);

if (customResource != null) {
node.typeDecl = customResource.type;
}

let attrs = node.attrs.sort((a, b) => {
var ai = this.localProvidors.indexOf(a.name);
var bi = this.localProvidors.indexOf(b.name);
Expand Down Expand Up @@ -136,7 +142,21 @@ export class BindingRule extends ASTBuilder {

switch (instructionName) {
case "BehaviorInstruction": {
this.examineBehaviorInstruction(node, <BehaviorInstruction>instruction);
var behaviorInstruction = <BehaviorInstruction>instruction;
this.examineBehaviorInstruction(node, behaviorInstruction);

if (node.typeDecl && node.typeDecl.kind == ts.SyntaxKind.ClassDeclaration && (<ts.ClassDeclaration>node.typeDecl).members) {
var members = (<ts.ClassDeclaration>node.typeDecl).members;

if (members.findIndex(x => x.name.getText() == behaviorInstruction.attrName) == -1) {
this.reportIssue(<Issue>{
message: `cannot find '${behaviorInstruction.attrName}' in type '${node.typeDecl.name.getText()}'`,
severity: IssueSeverity.Error,
line: attr.location.line,
column: attr.location.column
});
}
}
break;
}
case "ListenerExpression": {
Expand Down
2 changes: 1 addition & 1 deletion spec/ast.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ describe("Abstract Syntax Tree", () => {
expect(locals[0].type).toEqual(<ts.TypeNode>ts.createNode(ts.SyntaxKind.StringKeyword));
});
it("will create correct AST when void present", (done) => {
var builder = new ASTBuilder();
var builder = new ASTBuilder(new Reflection());
var linter: Linter = new Linter([
builder
]);
Expand Down
39 changes: 39 additions & 0 deletions spec/binding.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1555,6 +1555,45 @@ describe("Static-Type Binding Tests", () => {
});
});

//#67
describe("custom elements", () => {

it("should detect invalid bindable attributes on custom elements", (done) => {
let item = `
export class ItemCustomElement{
target:string;
}`;
const viewmodel = `
export class Foo{
myname:string;
}`;
const view = `
<template>
<require from='./item'></require>
<item target.bind="myname" missing.bind="alsoMissing"></item>
</template>`;
const reflection = new Reflection();
const rule = new BindingRule(reflection, new AureliaReflection());
const linter = new Linter([rule]);
reflection.add("./foo.ts", viewmodel);
reflection.add("./item", item);
linter.lint(view, "./foo.html")
.then((issues) => {
try {
expect(issues.length).toBe(2);
expect(issues[1].message).toBe("cannot find 'missing' in type 'ItemCustomElement'");
expect(issues[0].message).toBe("cannot find 'alsoMissing' in type 'Foo'");
}
catch (err) {
fail(err);
}
finally {
done();
}
});
});
});


/*it("rejects more than one class in view-model file", (done) => {
let viewmodel = `
Expand Down
4 changes: 2 additions & 2 deletions spec/repeatfor.spec.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@

import { Linter, Rule } from 'template-lint';
import { ASTBuilder } from '../source/ast';
import { Reflection } from '../source/reflection';

describe("RepeatFor Testing", () => {

var linter: Linter = new Linter([
new ASTBuilder()
]);
new ASTBuilder(new Reflection())]);

it("will pass item of items", (done) => {
linter.lint('<div repeat.for="item of items"></div>')
Expand Down

0 comments on commit 26316d4

Please sign in to comment.