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

Fix View-Model <-> View Resolution #113

Closed
atsu85 opened this issue Sep 14, 2016 · 11 comments
Closed

Fix View-Model <-> View Resolution #113

atsu85 opened this issue Sep 14, 2016 · 11 comments
Assignees
Labels
Milestone

Comments

@atsu85
Copy link
Contributor

atsu85 commented Sep 14, 2016

Let's say I have 2 files:

my-component.ts
my-component.html

and my-component.ts contains smth like:

export class Helper {
...
}
export class MyComponent {
...
  x: Helper;
}

and when field x is used in template, then linter emits error:

ERROR: cannot find 'x' in type 'Helper '

Right now the error message is misleading.

PS. While I think it is good idea to put component class as the first class in TS file, it shouldn't be a restriction for the linter (ideally there could be another rule that could be enabled/disabled to check if VM class is the first class in source code) - linter should match the VM class using same rules (naming conventions in this case) as it is done by Aurelia.

@MeirionHughes
Copy link
Contributor

I'm pretty sure I spoke to Rob about this a while back and the convention is Aurelia expects the vm to be the first exported class. i.e. you can change MyComponent to BlahBlah and it will still work.

@atsu85
Copy link
Contributor Author

atsu85 commented Sep 19, 2016

@MeirionHughes, that only applies for routable components, but not for custom elements. For example if I rename custom element class (that doesn't have annotation that changes the name of custom element tag) to smth else and try to reload the page, then custom element is not rendered.

And if I just add another class before custom element class, then custom element still works as expected, but template-lint produces the error i described above.

@EisenbergEffect - this seems a bit inconsistent, why should routable components (view's) act differently compared to custom elements?

@MeirionHughes MeirionHughes changed the title incorrect error messages when custom element class is not the first exported class in the TS file View-Model resolution for custom-element (non-route) is incorrect. Sep 19, 2016
@MeirionHughes
Copy link
Contributor

MeirionHughes commented Sep 19, 2016

hmm okay. There is the useView and noView decorators to contend with too. (view-model picking whatever view it wants). Need to think about this some more...

@jdanyow
Copy link

jdanyow commented Sep 27, 2016

May need to setup some of the scenarios and debug them. The ModuleAnalyzer and it's callers would be a good place to start.

@MeirionHughes MeirionHughes added this to the 0.10 milestone Sep 28, 2016
@MeirionHughes MeirionHughes changed the title View-Model resolution for custom-element (non-route) is incorrect. Fix View-Model Resolution Sep 28, 2016
@MeirionHughes MeirionHughes changed the title Fix View-Model Resolution Fix View-Model <-> View Resolution Sep 28, 2016
@MeirionHughes
Copy link
Contributor

MeirionHughes commented Dec 6, 2016

Main issue I have here is I don't (can't) know the context for with a html is used from; i.e. we lint a html file and try to work out what it is; aurelia's runtime has more to go on in this regard.

If the [html+ts] combo is a custom-element, then, to be used correctly anyway, the corresponding source file should have a class called SomethingCustomElement, if it doesn't them we'll have to assume the html file is part of a route and that the first exported class is the viewmodel.

So I'm going to go with:

"some-thing.html" -> "some-thing.[ts|js]" -> "SomeThing"

If "some-thing.[ts|js]" has class SomeThingCustomElement then use that (Custom Element)
If "some-thing.[ts|js]" first export is class "SomeThing" then use that (Custom Element)
If "some-thing.[ts|js]" first export is class "Anything" then use that (Route)

@MeirionHughes MeirionHughes self-assigned this Dec 6, 2016
@atsu85
Copy link
Contributor Author

atsu85 commented Dec 7, 2016

@MeirionHughes, please make the function that (decides if it is custom element or route view) configurable, so I could configure the linter based my naming conventions.

@MeirionHughes
Copy link
Contributor

MeirionHughes commented Dec 7, 2016

@atsu85 I can, but...

Can you make a gistrun of what you're doing differently? Your example in the first post doesn't work for me. https://gist.run/?id=c2a7e42b7d835923b2ec5f522d654d9b if I rename it to MyComponentCustomElement it works. if I change MyComponent to be the first export, it works. if I change it to class Anything as first export, it doesn't work as Custom Element.

The three rules I outlined above seem to be exactly how aurelia works by default.

@MeirionHughes
Copy link
Contributor

I think I need to change how the linter is used; currently it comes at the project from the html files then tries to find view-models / source; there are quite a few ways for that to go wrong. see https://github.com/MeirionHughes/aurelia-template-lint/issues/151

Perhaps I need to try to come at it from the source files; specifically the main / app entry points and then the router module paths. This is how aurelia does it (unless you require a html file directly). :/

@atsu85
Copy link
Contributor Author

atsu85 commented Dec 20, 2016

I think I need to change how the linter is used; currently it comes at the project from the html files then tries to find view-models / source;

Perhaps I need to try to come at it from the source files;

I agree, that current approach has uncovered corner-cases.

specifically the main / app entry points and then the router module paths.

But I'm not sure if it makes sense to over-complicate things like that. For example getViewStrategy method may dynamically compose html file name (the name might be computed using string concatenation, etc or even fetched from the server). Not sure how useful would the new approach be in this case. But to support @useView("foo.html") decorator, You could just scan all the classes (without trying to figure out what views/customElements are used from some or all application entrypoints).

Another corner-case that i suspect would be very difficult to cover is using compose tag, that could dynamically take any html template and viewModel.

@Alexander-Taran
Copy link

Probably can be closed
Not sure what Is the current roadmap for linter.
vscode/other ides only?
cmdline tool?

@EisenbergEffect
Copy link
Contributor

This will be covered by the work to incorporate the linter and extend the VS plugin and language server.

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

No branches or pull requests

5 participants