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

Type checking for template expressions #681

Merged
merged 55 commits into from
Apr 15, 2019

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 commented Feb 5, 2018

😄
template-type-checking

@ktsn
Copy link
Member Author

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';
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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';
Copy link
Member

Choose a reason for hiding this comment

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

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

@HerringtonDarkholme
Copy link
Member

@octref
Copy link
Member

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 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 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 minimized.

@ktsn

This comment has been minimized.

@ktsn
Copy link
Member Author

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 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 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
@ktsn ktsn deleted the template-type-checking branch April 15, 2019 17:41
@ktsn
Copy link
Member Author

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

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 commented Apr 26, 2019

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

@trusktr
Copy link

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 commented Aug 7, 2019

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

Successfully merging this pull request may close these issues.

Type checking for template expressions
10 participants