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.

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

StaticType Rule Bugs #35

Closed
18 tasks done
MeirionHughes opened this issue Jun 21, 2016 · 20 comments
Closed
18 tasks done

StaticType Rule Bugs #35

MeirionHughes opened this issue Jun 21, 2016 · 20 comments
Assignees
Milestone

Comments

@MeirionHughes
Copy link
Contributor

MeirionHughes commented Jun 21, 2016

Please report any issues you find and I will try to collate and fix them as I can.

  • Column offsets are always 0
  • Order of Attributes in same element can break bindings to a locals
  • Ancestor access doesn't check types i.e. $parent
  • Capitalised filename should resolve (limited to Foo_ViewModel_ currently)
  • Doesn't follow camel-case convention file <-> class
  • Doesn't support Mapping ex: repeat.for="[id, customer] of customers"
  • Complains it cannot find converter as field member
  • Should add $index, $odd $even inside repeat.for as known variables
  • context variables aren't overridden in nested controllers (same local name)
  • Doesn't recognise interfaces.
  • repeat.for doesn't work with interfaces imported from other files
  • Template is not checked if the view model class name is different
  • Traverses into private fields (should be warning).
  • Doesn't support interpolation with attribute value
  • Doesn't support Keyed Access
  • Doesn't support negation operator on if.bind
  • Bindable attributes passed to custom elements should be checked.
  • Custom elements should be checked.
  • Doesn't support Generics and use of Generic member types moved to feature
  • Doesn't support Value Converters (different Type output) moved to feature
@MeirionHughes MeirionHughes modified the milestones: 0.8, 0.7 Jun 22, 2016
@MeirionHughes MeirionHughes self-assigned this Jun 22, 2016
MeirionHughes added a commit that referenced this issue Jun 22, 2016
@zakjan
Copy link

zakjan commented Jun 23, 2016

repeat.for doesn't work with interfaces imported from other files

item.ts

export interface Item {}

page.ts

import {Item} from "./item";

export class PageViewModel {
    items: Item[];
}

page.html

<template>
    <template repeat.for="item of items">
        ${item}
    </template>
</template>

linter output:

[01:07:19] WARNING: cannot find 'item' in type 'PageViewModel' Ln 3 Col 0 /src/pages/page.html

@zakjan
Copy link

zakjan commented Jun 23, 2016

Template is not checked if the view model class name is different from the view and view model file names.

src/profiles/list.ts

export class ProfilesListViewModel {}

src/profiles/list.html

<template>
    ${item}
</template>

There is no linter issue found.

Also about your point "Doesn't follow camel-case convention file <-> class", I think no case convention should be enforced, the same basename of both files should be enough to pair them together.

@zakjan
Copy link

zakjan commented Jun 23, 2016

Custom elements should be checked.

item.ts

import {bindable} from "aurelia-templating";

export class ItemCustomElement {
    @bindable value: string;
}

item.html

<template>
    ${valu}
</template>

@zakjan
Copy link

zakjan commented Jun 23, 2016

Bindable attributes passed to custom elements should be checked.

item.ts

import {bindable} from "aurelia-templating";

export class ItemCustomElement {
    @bindable value: string;
}

item.html

<template>
    ${value}
</template>

page.ts

export class PageViewModel {
    value: number;
}

page.html

<template>
    <require from="./item"></require>

    <item value.bind="value"></item>
</template>

The same applies for value convertors, not only their output, but also their input should be checked.

@zakjan
Copy link

zakjan commented Jun 24, 2016

An idea to think about: allow only strings (and maybe numbers) in string interpolations?

export class PageViewModel {
    item: {name: string};
}

<template>
    <!-- fail -->
    ${item}
    <div title="Item: ${item}"></div>
    <!-- ok -->
    ${item.name}
    <div title="Item: ${item.name}"></div>
</template>

It could prevent unintentional implicit casting, for example to "[object Object]".

@atsu85
Copy link
Contributor

atsu85 commented Jun 24, 2016

I'd certainly use that.

@MeirionHughes
Copy link
Contributor Author

MeirionHughes commented Jun 24, 2016

...allow only strings (and maybe numbers) in string interpolations?

yeah that's good idea; could you lift that out as a feature request? I don't think its a bug per se.

@zakjan
Copy link

zakjan commented Jun 24, 2016

Yeah, of course. #44

MeirionHughes added a commit that referenced this issue Jun 25, 2016
MeirionHughes added a commit that referenced this issue Jun 26, 2016
fix minor bugs.
#35
@zakjan
Copy link

zakjan commented Jun 27, 2016

Boolean not operator reports error.

export class PageViewModel {
    value: string;
}

<template>
    <template if.bind="!value">
        ...
    </template>
</template>

linter output:

[20:48:18] WARNING: cannot find 'undefined' in type 'PageViewModel' Ln 2 Col 0 /src/pages/profiles/create/page.html

@MeirionHughes MeirionHughes modified the milestones: 0.8, 0.7 Jun 28, 2016
MeirionHughes added a commit that referenced this issue Jun 28, 2016
ast testing
fix inheriting locals ref: #35
@MeirionHughes
Copy link
Contributor Author

with regard to attribute ordering and using a local before it has been declared;
the issue is this:

<option model.bind = "employee.id" repeat.for="employee of employees"  > 

which can be fixed by using a more (in my view) logical ordering:

<option repeat.for="employee of employees"  model.bind = "employee.id" > 

given there are so many issues to work though, I won't be fixing this as yet.

@atsu85
Copy link
Contributor

atsu85 commented Jun 30, 2016

which can be fixed by using a more (in my view) logical ordering:

@EisenbergEffect, do I remember correctly, that different browsers gibe attributes in different order? If I remember correctly, then one browser gave it in the order it was written in source code, and some other browser gave it alphabetically? If so, then we shouldn't rely on the order of attributes, but rather set them on different elements?

@EisenbergEffect
Copy link
Contributor

Yes. I don't think you can rely on attribute order. That's one reason why you can't have two template controllers on the same element and have to nest a template.

@EisenbergEffect
Copy link
Contributor

But, the above example should work because the template controller overrides everything else and causes the scope to be created.

@atsu85
Copy link
Contributor

atsu85 commented Jun 30, 2016

@EisenbergEffect, I don't understand Your last message.

Do You mean, that the order of repeat.for and model.bind shouldn't matter, as

template controller overrides everything else and causes the scope to be created.

(that would contradict what @MeirionHughes mentioned earlier)
or do You mean, that in case repeat.for comes before model.bind it should work (as @MeirionHughes said)?

@EisenbergEffect
Copy link
Contributor

The order should not matter. The presence of a template controller overrides everything else on the element. However, it makes more sense to have the template controller first.

MeirionHughes added a commit that referenced this issue Jun 30, 2016
support custom typings.
ref #35
closes #36
MeirionHughes added a commit that referenced this issue Jun 30, 2016
@MeirionHughes
Copy link
Contributor Author

MeirionHughes commented Jun 30, 2016

Resolved; realized I could simply reorder them now that I pre-process into an AST.

MeirionHughes added a commit that referenced this issue Jul 1, 2016
update README.
fix type checking of method return type
support javascript sources. closes #52
Migrate conflicting-attribute. closes #32
ensure unknown converters fail silently.
Check bindable field is okay. ref #35
Support Syntax Rule configuration
Support any attribute order for controllers. ref #35
Support Keyed Access.  ref #35
Fix negated if.bind
support interpolation within binding.
support custom typings. closes #36
reject access to private fields. ref: #35
support ancestor scope with correct context ref: #35
fix view-model resolution. ref: #35
fix inheriting locals ref: #35
update and unlock versions
improve unhandled error msg
replace repeatfor. closes #40
fix view-model not found
rework static-type rule closes #33
MeirionHughes added a commit that referenced this issue Jul 1, 2016
update README.
fix type checking of method return type
support javascript sources. closes #52
Migrate conflicting-attribute. closes #32
ensure unknown converters fail silently.
Check bindable field is okay. ref #35
Support Syntax Rule configuration
Support any attribute order for controllers. ref #35
Support Keyed Access.  ref #35
Fix negated if.bind
support interpolation within binding.
support custom typings. closes #36
reject access to private fields. ref: #35
support ancestor scope with correct context ref: #35
fix view-model resolution. ref: #35
fix inheriting locals ref: #35
update and unlock versions
improve unhandled error msg
replace repeatfor. closes #40
fix view-model not found
rework static-type rule closes #33
@atsu85
Copy link
Contributor

atsu85 commented Jul 2, 2016

So this is the first feature, that modifies the output compared to the input?

@MeirionHughes
Copy link
Contributor Author

MeirionHughes commented Jul 2, 2016

Nope, nothing changes. The analysis code is basically one big state machine; I've simply re-ordered the attributes in the Syntax Tree, so the template controllers are examined first; presumably only the controllers add local variables. This means if you have <option model.bind = "employee.id" repeat.for="employee of employees" > in your code; it will correctly handle model.bind and know about employee.

The linter (with gulp) does nothing to the output.

@niieani
Copy link

niieani commented Nov 12, 2016

Linting the navigation-skeleton with StaticType analysis shows the error:

WARNING in ./src/users.html
[9, 11]: cannot find 'image' in type 'Users'

 @ ./src/users.ts 95:0-31
 @ ./src/app.ts
 @ ./src/main.ts
 @ multi app

The source of this is blur-image.bind="image":

<canvas class="header-bg" width="250" height="70" blur-image.bind="image"></canvas>
<div class="avatar">
  <img src.bind="user.avatar_url" crossorigin ref="image"/>
</div>

The thing is, it's binding to the ref on the img below. Also, the whole thing is in a repeat.for="user of users" loop, so image property belongs on the IUser interface, not on the Users view-model.

@RomkeVdMeulen
Copy link

I'm getting false positives on vars added through ref like @niieani mentioned. Also a lot of false positives on things added through aurelia-validation's validation-errors attribute.

<form id="new" validation-errors="errors.bind: newErrors; controller.bind: newValidationController">
	<input type="text" value.bind="new.name & validate"/>
	<input type="submit"/>

	<p repeat.for="newError of newErrors" class="warning">
		${newError.error.message}
	</p>
</form>

gives:

[11:07:15] WARNING: cannot find 'newErrors' in type 'MyEditor' Ln 5

The simple binding validation-errors.bind="errors" also gives these false positives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants