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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Type checking for template expressions #681

Merged
merged 55 commits into from Apr 15, 2019
Merged

Conversation

@ktsn
Copy link
Member

@ktsn ktsn commented Feb 5, 2018

This is work in progress PR which enables Vetur to type check template expressions.

The approach is the same as I wrote in #209 - transforming template html to TypeScript AST, then type checking it. I use vue-eslint-parser to transform HTML string to AST as it considers vue specific things like directives. I wrote the transformer that converts ESLint AST to TypeScript AST. Note that the output TypeScript AST does not make sense in runtime to simplify the implementation as it is meant to be used for extracting type info from template expression.

With this feature, we may also able to provide completion in template and component props type checking but I would like to leave them for another PR since this PR is already large without them 馃槄

BTW, should we disable this feature in default and let the users enable it by configuration?

TODO for this PR

  • A overall good structure
  • Good test coverage
  • Update diagnostics when script block is updated
  • Add a new option vetur.experimental.templateTypeCheck

Remaining Tasks (after this PR)

  • Check other directives expression. (currently only works in v-bind/v-on)
  • Support filter
  • Support scoped slot
  • Suppress An object literal cannot have multiple properties with the same name in strict mode. error.
    • e.g. <div class="foo" :class="bar"></div> provides this error.
  • Support type narrowing by v-if

fix #209

@ktsn
Copy link
Member Author

@ktsn ktsn commented Feb 5, 2018

馃槃
template-type-checking

@ktsn
Copy link
Member Author

@ktsn ktsn commented Feb 5, 2018

It probably be better to see diff via https://github.com/vuejs/vetur/pull/681/files?w=1 when looking into serviceHost.ts since there is an indentation change.

@@ -1,65 +1,111 @@
import * as ts from 'typescript';
import * as path from 'path';
import { parse } from 'vue-eslint-parser';

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Feb 6, 2018
Member

We can implement parsing in VLS at all, so no additional dependency/script is needed. Actually template completion needs it too.

But I don't have time to implement Vue specific elements 馃槥 . For now eslint-parser is the only option.

This comment has been minimized.

@ktsn

ktsn Feb 7, 2018
Author Member

Yes. I actually tried to extend the parser in VLS on the first time but I ended up using vue-eslint-parser because I would like to focus finishing essential implementation of template type checking at first.
In that case, I can work on the VSL template parser after this PR is finished 馃檪

import * as ts from 'typescript';
import { AST } from 'vue-eslint-parser';

export const renderHelperName = '__veturRenderHelper';

This comment has been minimized.

@HerringtonDarkholme

HerringtonDarkholme Feb 6, 2018
Member

Renaming it to __vlsRenderHelper is better since vue-language-server is designed to be client agnostic.

@octref
Copy link
Member

@octref octref commented Feb 7, 2018

First -- great work! Thanks for pushing this forward! 馃帀

BTW, should we disable this feature in default and let the users enable it by configuration?

Yes. We should put an option like vetur.experimental.templateTypeCheck before it.

But I think currently the biggest problem is that there is no longer a clean separation of mode, as the type checking exists on both template/script modes. If we want to do suggestion/hovering and other LS features, it makes sense to create a templateInterpolation mode.

TODO

Let's aim for a MVP, where we have:

  • A overall good structure
  • Good test coverage
  • Update diagnostics when script block is updated

Can you add me to your fork so I can push changes? I'll have some time to look deeper into this PR this weekend. Before that I'll spend some time looking into some other smaller, long-standing bugs...

@HerringtonDarkholme Yeah I agree we should start adding some integration tests running VLS against real-world repos. This is one of the things I plan to look into this month.

@octref
Copy link
Member

@octref octref commented Apr 10, 2019

I fixed some tests related to upgrading Vue types.
There are 2 more failing tests which I believe is caused by TS Server trying to resolve the .vue.template files for definition and references.

@octref octref closed this Apr 10, 2019
@octref octref reopened this Apr 10, 2019
@octref
Copy link
Member

@octref octref commented Apr 10, 2019

I see, the problem is you try to set zero pos for all the template-script code, so TS resolved them to invalid locations and throws there.

I think we can try maybe appending these lines to the end of file and map them back? This way we can get other features such as hover, jump to definition (from template), etc working as well. I'll give it a try.

@ktsn

This comment has been hidden.

@ktsn

This comment has been hidden.

@ktsn
Copy link
Member Author

@ktsn ktsn commented Apr 13, 2019

  • Fix the todos in transformTemplate. Especially the ones concerning VExpressionContainer | VIdentifier
  • Fix the crash on v-on.vue.

Fixed them.

I also tweaked template transformer a bit to avoid false-positive diagnostics about v-slot value 2923e08. While it just ignore the v-slot but we should include the declared variables into scope. I'll work on it a later PR.

@ktsn ktsn changed the title [WIP] Type checking for template expressions Type checking for template expressions Apr 13, 2019
@octref
Copy link
Member

@octref octref commented Apr 13, 2019

Never mind about the above comment. If I quit all VSCode process, the test works.

Yeah, if you are testing against VS Code stable, having another VS Code stable open would cause issues. I should have surfaced the error though.

@octref
Copy link
Member

@octref octref commented Apr 15, 2019

Merged. @ktsn Congratulations 馃帀 馃憤

Do you mind creating a new issue to track what you plan to do? I can add to that issue my side of things.

From my perspective I'd like to see if I can make completion work from the .template files, and I'd want to implement a mapping between the .template files and the Vue templates in SFC so we get hover, jump to definition, find references etc working.

@octref octref merged commit 782d5b6 into vuejs:master Apr 15, 2019
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@ktsn ktsn deleted the ktsn:template-type-checking branch Apr 15, 2019
@ktsn
Copy link
Member Author

@ktsn ktsn commented Apr 15, 2019

@octref Finally 馃帀 Thank you for cooperating on this!
Sure, I'll create new issues for remaining tasks.

hover, jump to definition, find references etc working

Yeah, they are also promising features. I'm so excited we will have them 馃槃

@octref octref added this to the April 2019 milestone Apr 16, 2019
@mpvosseller
Copy link

@mpvosseller mpvosseller commented Apr 25, 2019

This is great! Is there any way to integrate this work into command line builds to get the same errors/warnings there?

@octref
Copy link
Member

@octref octref commented Apr 26, 2019

@mpvosseller I updated #1149, you can subscribe to it.

@guyguy2001 guyguy2001 mentioned this pull request May 2, 2019
3 of 3 tasks complete
@trusktr
Copy link

@trusktr trusktr commented Jul 24, 2019

Hello, slightly off topic, but I'm interested in making my own template system. F.e. I'd like to use @if="" instead of v-if="", or something.

What would be involved in order to make this type checking system work with my own (but similar) syntax?

@octref
Copy link
Member

@octref octref commented Aug 7, 2019

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

Successfully merging this pull request may close these issues.

10 participants